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

Feature: Immutable build #172

Merged
merged 11 commits into from
Jul 16, 2017
Merged

Feature: Immutable build #172

merged 11 commits into from
Jul 16, 2017

Conversation

thangngoc89
Copy link
Collaborator

@thangngoc89 thangngoc89 commented Jul 1, 2017

I'm pushing this here to get the ideas first, will fix test later
With this change, only index.html and sw.js is mutable between builds.

Fixes #23
Blocked by #162

@developit
Copy link
Member

We should pull some people in regarding the hashing, I think it has kinda wild implications for ServiceWorkers.

@thangngoc89
Copy link
Collaborator Author

I'm not sure but never see anyone hash ServiceWorker runtime, just be sure to set the right HttpCache on your server.

@lukeed
Copy link
Member

lukeed commented Jul 1, 2017

You mean for style.css and bundle.js? Looks okay to me, what would be the problem? As per #23, they get treated like any other assets.

@thangngoc89
Copy link
Collaborator Author

thangngoc89 commented Jul 1, 2017

@rkostrzewski Pinging the tests guy. Since filename is changing between builds, we can't compare the assets anymore

@developit
Copy link
Member

We could take a page from CRA and strip the hashes from filenames prior to doing the comparison

@lukeed
Copy link
Member

lukeed commented Jul 1, 2017

Yup. That, or look for exact match else regex match.

@thangngoc89
Copy link
Collaborator Author

@developit if that is the case, I'll wait for #162 before fix the tests

@@ -220,7 +220,7 @@ export default (env) => {
// produce HTML & CSS:
addPlugins([
new ExtractTextPlugin({
filename: 'style.css',
filename: isProd ? "style.[contenthash:5].css" : "style.css",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love the idea on client, not so on server side. I'd much prefer to have static file on server for critical css inlining.

I think we should pass an argument to webpack-base-config from webpack-client-config and webpack-server-config

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other option is to expose a mainfest.json which would be used to resolving filenames from entries (dunno how this works - only seen folks doing that)

@rkostrzewski
Copy link
Collaborator

@thangngoc89 I'll merge the tests fix and add regex matching - that sounds cool 😎

@rkostrzewski
Copy link
Collaborator

Travis build failed because it got different hashes than on my local env. This could break long term hashing.

Different hashes = some code is undeterministic (i.e. relies on env it was built on). My bet is that this line produced this output.

@thangngoc89
Copy link
Collaborator Author

@rkostrzewski it's not just that line, a lot of things need to to config to support long term caching

@rkostrzewski
Copy link
Collaborator

@thangngoc89 ok I've found what's the issue here. Entrypoint relative path changes on travis because of different way preact-cli is used ('global' vs local). After changes in async-component-loader entrypoints chunkhashes are consistend when using locally installed preact-cli (can't say the same about route-*.chunks 😢)

@thangngoc89
Copy link
Collaborator Author

thangngoc89 commented Jul 2, 2017

@rkostrzewski I'm suggesting that we keep this PR as starting point for immutable build. ANd send another PR for adding Long term caching support. (keep the hashes consistent between builds)

Copy link
Collaborator

@rkostrzewski rkostrzewski left a comment

Choose a reason for hiding this comment

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

LGTM. However, I'd like someone to review changes to async-component-loader & tests stuff as I've made them 😜

weight: 0.9
}
};
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed those spaces for you 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rkostrzewski Thx. That's usually prettier's job

@thangngoc89
Copy link
Collaborator Author

@lukeed @developit It's good for your review

@rkostrzewski rkostrzewski requested a review from lukeed July 8, 2017 09:30
@lukeed
Copy link
Member

lukeed commented Jul 10, 2017

I'll review this tonight 🙏

@reznord reznord merged commit dcde1e2 into master Jul 16, 2017
@reznord reznord deleted the feat/immutable-build branch July 16, 2017 13:34
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.

Version bundle.js & style.css
5 participants