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
Initial service worker #140
Conversation
return { | ||
name: "consts-plugin", | ||
async resolveId(id) { | ||
if (id !== "consts:") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ending both these new plugins with :
to signify they're something other than node_modules
. Open to better suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m okay with this.
output: { | ||
dir: "dist", | ||
format: "amd", | ||
sourcemap: true, | ||
entryFileNames: "[name]-[hash].js", | ||
entryFileNames: "[name].js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want service worker to have a hash. This also removes the hash from bootstrap, but we always inline it so it's ok. I mentioned this issue to the Rollup folks rollup/rollup#2585 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea of hiding that we are loading until we need to admit it. And the auto-reload is a great idea.
Couple of nits and Qs. Otherwise good to go.
input: "src/bootstrap.ts", | ||
input: { | ||
bootstrap: "src/bootstrap.ts", | ||
sw: "src/sw/index.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the SW a separate endpoint instead of using the same trick we are using for the Worker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The worker way gives us a hashed file name 😢
} | ||
return Object.entries(consts) | ||
.map( | ||
([key, value]) => `export const ${key} = ${JSON.stringify(value)};` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn’t this just be
export ${JSON.stringify(consts)}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, that's invalid syntax. You could do export default ${JSON.stringify(consts)}
, but then usage can't be minified or tree-shaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL. I thought when you do
const a = 1;
const b = 2;
export {a, b};
that the export is an object literal. It is not :(
}; | ||
|
||
private _gameChangeSubscribers = new Set<GameChangeCallback>(); | ||
private _awaitingGameTimeout: number = -1; | ||
private _stateService?: Remote<StateService>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you move this from props to an instance variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in state previously. Updating state causes a re-render, but we never want to update rendering when this value changes. We update rendering when game
changes, not when the means to create a game become available.
src/services/preact-canvas/index.tsx
Outdated
|
||
this._awaitingGameTimeout = setTimeout(() => { | ||
this.setState({ awaitingGame: true }); | ||
}, 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these kind of numbers to constants.ts
?
@@ -0,0 +1,12 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file isn’t needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is needed (as discussed in person)
@@ -11,5 +11,6 @@ | |||
"moduleResolution": "node", | |||
"baseUrl": "./", | |||
"types": [] | |||
} | |||
}, | |||
"exclude": ["src/sw/**/*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn’t needed.
@@ -0,0 +1 @@ | |||
import "../missing-types"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file isn’t needed.
Ready for review!
The service worker will install during dev. Add
?no-sw
to the end of the page URL to disable it.I spent a while trying to find a good place for an "update available" message, but then I thought: can we avoid it? In this PR I don't show a message, but if the user presses "start" when there's a update available, it'll perform the update then start the game.
I refactored our loading state as part of this. We were giving too much away by disabling/dimming the start button. The user can press start whenever they want, we only have to admit we aren't ready if they beat us to it. I implemented that flow in this PR.