Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add matchFilenameOnly start option #337

Conversation

msutkowski
Copy link
Member

@benmonro
Copy link
Contributor

@msutkowski thank you! this looks great to me.

@benmonro
Copy link
Contributor

@kettanaito any thoughts on this? Would really make your testcafe user's experience with testcafe a lot better. 😎

* you are using proxies and having difficulty to get the origin and `scriptURL` to match due to port differences.
* Defaults to `false`.
*/
matchFilenameOnly?: boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the issue and I think this solves that use case (and probably others too).
Another option would be to give the user this responsibility, which would cover all use cases.
It would be more complicated for the user, but it would be more flexible.

I'm not sure it needs to be more flexible tho. Perhaps this is something to keep in mind for when another use case pops up, before adding other options.

e.g.

serviceWorkerFileMatcher?: (scriptUrl:string) => boolean

if (serviceWorkerFileMatcher) {
  return serviceWorkerFileMatcher(worker.scriptURL)
}
return worker.scriptURL === absoluteWorkerUrl

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for this idea! I'm a big fan of matcher funcs as well (in this instance, as well as basically any other time it's relevant). I think the only real distinction is that what I'm proposing is the viewpoint: 'as a library, we've thought of what you're going to run into, and this is the easiest way to solve it', and the flag becomes obvious. The matcher func requires slightly more 🧠 power and documentation, but I'd agree that it is probably the better long-term solution.

My only challenge to the matcher func is basically what you said: "not sure it needs to be more flexible". That being said, I'm more than happy to change this to your suggestion and add in the tests once we have a solid decision 🎉.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should lean towards expanding API outwards (delegating more control to the user), not inwards (encapsulating specific behaviors under flags). Thanks for pointing this out, @timdeschryver!

Depending on when such matcher would be called, we can expose more information about the worker. Ideally, a ServiceWorkerRegistration reference, which contains about all you may need to create a custom registration predicate.

@msutkowski msutkowski closed this Aug 24, 2020
@kettanaito
Copy link
Member

Hey, @msutkowski 👋 Sorry for not getting a proper review on this one. Please, is there anything I can help with this pull request? The intention behind it definitely has a place to be, I don't see a custom worker registration predicate as an issue, especially if it helps to solve a problem.

@msutkowski
Copy link
Member Author

@kettanaito Sorry, I should've left a comment when closing. I'll be opening a new PR this afternoon unless you'd prefer the updates done in this branch/PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants