refactor: Removing unnecessary build outputs #1499
Conversation
🦋 Changeset detectedLatest commit: 392dcc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| expect(keys).toHaveLength(Object.keys(src).length); | ||
| expect(Object.keys(src)).toHaveLength(keys.length); |
There was a problem hiding this comment.
This was backwards which made the test failure output rather confusing.
There was a problem hiding this comment.
When removing 200.html this line needs to fallback to index.html
https://github.com/preactjs/preact-cli/blob/master/packages/cli/sw/index.js#L23
packages/cli/sw/index.js
Outdated
| setCatchHandler(({ event }) => { | ||
| if (isNav(event)) { | ||
| return caches.match(getCacheKeyForURL('/200.html')); | ||
| return ( | ||
| caches.match(getCacheKeyForURL('/200.html')) || | ||
| caches.match(getCacheKeyForURL('/index.html')) | ||
| ); | ||
| } |
There was a problem hiding this comment.
@prateekbh Sorry, missed that. Hopefully this will work?
There was a problem hiding this comment.
nvm, need to at least touch up the test suite first
There was a problem hiding this comment.
Alright, should be corrected now.
There was a problem hiding this comment.
sry for late reply would caches.match(getCacheKeyForURL('/200.html') || getCacheKeyForURL('/index.html')) work?
There was a problem hiding this comment.
No worries. Should do. Not sure why I did it like that.
There was a problem hiding this comment.
Corrected. Thanks for the catch.
|
@rschristian Does this PR require any more changes to be merged? Thanks for the contribution, I am new to preact, but would love to see this land. |
Looks like I need to rebase the changes but otherwise that'd be up to Prateek/anyone else who had improvements/concerns. |
rschristian
force-pushed
the
refactor/pruneUnnecessaryBuildOutputs
branch
from
43223b5 to
392dcc5
Compare
|
@rschristian as a follow up can you add a test similar to https://github.com/preactjs/preact-cli/blob/master/packages/cli/tests/service-worker.test.js#L48-L63 but with pre-rendering disabled |
|
@prateekbh Sorry, hadn't gotten the chance to add a test yet due to the holidays. Figured I'd pick it back up on Monday (probably) |
|
Waiting for release |
What kind of change does this PR introduce?
Removing unnecessary files from being output by build
Did you add tests for your changes?
Edited existing
Summary
Closes #1495
Does this PR introduce a breaking change?
No