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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Add matchFilenameOnly start option #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4c01a5
Add matchFilenameOnly start option
msutkowski Aug 14, 2020
5f20d29
Match coding style
msutkowski Aug 14, 2020
2ad68b3
Fix matchFilenameOnly flag description
msutkowski Aug 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"scripts": {
"start": "cross-env NODE_ENV=development rollup -c rollup.config.ts -w",
"clean": "rimraf lib node/**/*.js",
"lint": "eslint '{cli,config,src,test}/**/*.ts'",
"lint": "eslint \"{cli,config,src,test}/**/*.ts\"",
"build": "yarn clean && cross-env NODE_ENV=production rollup -c rollup.config.ts",
"test": "yarn test:unit && yarn test:integration",
"test:unit": "cross-env BABEL_ENV=test jest --runInBand",
Expand Down
8 changes: 8 additions & 0 deletions src/setupWorker/glossary.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ export type StartOptions = SharedOptions & {
* instance is ready. Defaults to `true`.
*/
waitUntilReady?: boolean

/**
* A boolean that allows you to override the default strict matching behavior. If set to `true`, the first worker
* based on a matching filename only (ex: mockServiceWorker.js) will be returned. This is primarily useful in the scenario that
* 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.

}

export type RequestHandlersList = RequestHandler<any, any>[]
Expand Down
2 changes: 2 additions & 0 deletions src/setupWorker/start/createStart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const DEFAULT_START_OPTIONS: DeepRequired<StartOptions> = {
quiet: false,
waitUntilReady: true,
onUnhandledRequest: 'bypass',
matchFilenameOnly: false,
}

export const createStart = (context: SetupWorkerInternalContext) => {
Expand Down Expand Up @@ -50,6 +51,7 @@ export const createStart = (context: SetupWorkerInternalContext) => {
getWorkerInstance(
resolvedOptions.serviceWorker.url,
resolvedOptions.serviceWorker.options,
resolvedOptions.matchFilenameOnly,
),
)

Expand Down
5 changes: 5 additions & 0 deletions src/setupWorker/start/utils/getWorkerByRegistration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
export const getWorkerByRegistration = (
registration: ServiceWorkerRegistration,
absoluteWorkerUrl: string,
matchFilenameOnly = false,
): ServiceWorker | null => {
const allStates = [
registration.active,
Expand All @@ -13,6 +14,10 @@ export const getWorkerByRegistration = (
]
const existingStates = allStates.filter(Boolean) as ServiceWorker[]
const mockWorker = existingStates.find((worker) => {
if (matchFilenameOnly) {
const workerFileName = absoluteWorkerUrl.split('/').pop()
return workerFileName ? worker.scriptURL.includes(workerFileName) : false
}
return worker.scriptURL === absoluteWorkerUrl
})

Expand Down
21 changes: 17 additions & 4 deletions src/setupWorker/start/utils/getWorkerInstance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { until } from '@open-draft/until'
import { getWorkerByRegistration } from './getWorkerByRegistration'
import { ServiceWorkerInstanceTuple } from '../../glossary'
import { ServiceWorkerInstanceTuple, StartOptions } from '../../glossary'
import { getAbsoluteWorkerUrl } from '../../../utils/getAbsoluteWorkerUrl'

/**
Expand All @@ -10,14 +10,19 @@ import { getAbsoluteWorkerUrl } from '../../../utils/getAbsoluteWorkerUrl'
export const getWorkerInstance = async (
url: string,
options?: RegistrationOptions,
matchFilenameOnly?: StartOptions['matchFilenameOnly'],
): Promise<ServiceWorkerInstanceTuple | null> => {
// Resolve the absolute Service Worker URL
const absoluteWorkerUrl = getAbsoluteWorkerUrl(url)

const [, mockRegistrations] = await until(async () => {
const registrations = await navigator.serviceWorker.getRegistrations()
return registrations.filter((registration) => {
return getWorkerByRegistration(registration, absoluteWorkerUrl)
return getWorkerByRegistration(
registration,
absoluteWorkerUrl,
matchFilenameOnly,
)
})
})

Expand All @@ -37,7 +42,11 @@ export const getWorkerInstance = async (
// Update existing service worker to ensure it's up-to-date
return existingRegistration.update().then(() => {
return [
getWorkerByRegistration(existingRegistration, absoluteWorkerUrl),
getWorkerByRegistration(
existingRegistration,
absoluteWorkerUrl,
matchFilenameOnly,
),
existingRegistration,
]
})
Expand All @@ -49,7 +58,11 @@ export const getWorkerInstance = async (
return [
// Compare existing worker registration by its worker URL,
// to prevent irrelevant workers to resolve here (such as Codesandbox worker).
getWorkerByRegistration(registration, absoluteWorkerUrl),
getWorkerByRegistration(
registration,
absoluteWorkerUrl,
matchFilenameOnly,
),
registration,
]
},
Expand Down