feat(build): allow output hashing to be configured #3885

Merged
merged 4 commits into from Jan 10, 2017

Projects

None yet

5 participants

@clydin
Contributor
clydin commented Jan 6, 2017 edited

This allows the output filename hashes to be configured during a build via a new build command option --output-hashing. There are four possible values:

  • none: no hashing performed
  • media: only add hashes to files processed via [url|file]-loaders
  • bundles: only add hashes to the output bundles
  • all: add hashes to both media and bundles

none is the default for the development target.
all is the default for the production target.

Fix #2774

@clydin clydin feat: allow output hashing to be configured
47820b8
@googlebot googlebot added the cla: yes label Jan 6, 2017
+
+const hashFormats: { [option: string]: any } = {
+ 'none': { chunk: '', extract: '', file: '[name]' },
+ 'media': { chunk: '', extract: '', file: '[hash:20]' },
@tsm91
tsm91 Jan 6, 2017

hashlength note: I think a 20 character long hash is unnecessary and make it much more less human readable. 6 would be much better.

media (file) template format note: as stated in #3871 (comment). sky-bg.jpg becomes 8bcfb98e274f484ebac02d81670b7e4c.jpg with '[hash:20]'. Don't you think [name].[hash:20] would be more ideal? Looking at 8bcfb98e274f484ebac02d81670b7e4c.jpg have absolutely no idea which image it is, that is a problem is think.

@clydin
clydin Jan 7, 2017 edited Contributor

20 is the webpack default and the existing tests rely on it so I went with it for consistency.
Personally I'd prefer a small decrease in human readability in exchange for a lower chance of collision. Especially considering they're not intended to be overly human readable. (we have dev builds for that.)
How about something in the middle: 12?
You're example is actually [hash:32] or just[hash] when using the file-loader. Oddly, [hash] means different things in different places in webpack and the defaults don't seem to carry over either.
I agree on the name inclusion as it would be consistent with the others but was hesitant to change the existing paradigm without a consensus.

@filipesilva thoughts?

@tsm91
tsm91 Jan 7, 2017

How about something in the middle: 12?

Sounds good to me if you dont mind also editing the test cases.

I agree on the name inclusion as it would be consistent with the others but was hesitant to change the existing paradigm without a consensus.

Lets get a green light from one of the maintainers. File- and url-loader should emit a filename containing [name].[hash:12] or [name].[hash:20] for consistency and human readability.

@filipesilva
filipesilva Jan 7, 2017 Member

Regarding hash size, I think it's better to be consistent with the rest of the hashes already present so 20 sounds good to me.

The question of including the name is less obvious for me though. Having the name would be a change in the previous behaviour, but if we look at it with fresh eyes I think it's simply a better option since now everything that's processed follows the same pattern of [name].[hash].ext.

Some users are confused by the multitude of files suddenly propping up in their dist folder and that would help them make sense of it. It also adds a bit more collision protection.

So my vote goes for [name].[hash:20]. I think [name] should be in the common config instead of here though (like the others).

@filipesilva

Tests are missing as you mentioned but I think this is a really good PR.

It adds functionality users have been asking for, expands upon the fix itself to offer granular support and eliminates code duplication we had before due to hardcoded values.

Great work @clydin and @tsm91!

PS.: I hope you don't mind but I edited the PR comment to include the issue this PR fixes.

- new ExtractTextPlugin({filename: '[name].bundle.css'})
- ]
- };
+ return { };
@filipesilva
filipesilva Jan 7, 2017 Member

Gotta say I love the fact that this change makes the dev config dispensable. Can you remove it from the webpack merge procedure as well?

@clydin
clydin Jan 7, 2017 edited Contributor

I was planning on making a PR that adds NamedModulesPluginto dev builds to support HMR. Can do it another way though (e.g., only when HMR is enabled), if you'd prefer the file removed.

@filipesilva
filipesilva Jan 7, 2017 Member

If there's a reason for the file to exist then it shouldn't be removed. I didn't know of NamedModulesPlugin but a quick google search made me think it's main use is to have good names in HMR, is that correct? If so, it makes sense to have it automatically with HMR, yes.

@clydin
clydin Jan 7, 2017 edited Contributor

That's the reason I was going to add it. But it also stabilizes the module id's between builds which has benefits for cache busting. The hash won't change due to webpack module id renumbering. Nothing bad happens without it; the hash just might change more often than needed. And this isn't really relevant for HMR and probably not for dev builds in general.
For production builds there is HashedModuleIdsPlugin, which hashes the names to obfuscate the file path details.

@filipesilva
filipesilva Jan 7, 2017 Member

I'd be interested in such a PR then. Ping me if you end up doing it and I will review it.

@filipesilva
filipesilva Jan 7, 2017 Member

Also wonder if that would help with the reload times tbh. It sounds like it might do away with rebuilding some modules due to id changes.

@clydin
clydin Jan 7, 2017 Contributor

Not sure. But I'll see if I can put a PR together.
Also, I was looking through webpack-config.ts where the merging happens and to cleanly remove the dev config will require some refactoring (which the file could probably use either way) so it might be best to leave it for a followup PR.

@filipesilva
filipesilva Jan 7, 2017 Member

I want to refactor the way we pass options down to the webpack config anyway, so I can do that at the time. It's getting messy as is.

+
+const hashFormats: { [option: string]: any } = {
+ 'none': { chunk: '', extract: '', file: '[name]' },
+ 'media': { chunk: '', extract: '', file: '[hash:20]' },
@filipesilva
filipesilva Jan 7, 2017 Member

Regarding hash size, I think it's better to be consistent with the rest of the hashes already present so 20 sounds good to me.

The question of including the name is less obvious for me though. Having the name would be a change in the previous behaviour, but if we look at it with fresh eyes I think it's simply a better option since now everything that's processed follows the same pattern of [name].[hash].ext.

Some users are confused by the multitude of files suddenly propping up in their dist folder and that would help them make sense of it. It also adds a bit more collision protection.

So my vote goes for [name].[hash:20]. I think [name] should be in the common config instead of here though (like the others).

@filipesilva
Member

Regarding #3853, it actually asks for something slightly different. That request is for scripts/styles marked as lazy to never have hashes.

Personally I don't know of a way to exclude some files from having the hash, nor think it needs to be together in this PR. But if any of you know a way to do it I'd appreciate being pointed in the right direction and then I can make a followup PR with that fix (or you can, if you want to).

@clydin clydin cleanup and address comments
e8bb961
@tsm91
tsm91 commented Jan 9, 2017 edited

@filipesilva im not sure how this lazy option works in angular-cli, wanted to search the readme.md but got 0 results for the word lazy :D. Only do lazy route module loads with loadChildren, but that is a different story.

What i know is the AOT build output files will get hashed or non hashed the same way. It doesnt matter if its an AOT or non AOT build, what matters is if it is a dev or prod environment build: https://github.com/angular/angular-cli/blob/master/packages/angular-cli/models/webpack-config.ts#L53-L58

Just did an aot build on a fork with cache-bust disabled ng build --aot --prod --no-sourcemap --verbose --dcb and got the desired output: http://i.imgur.com/prcUbG0.png . Ofc in my fork the url-, file-loader still generates hashed media files.

.. nor think it needs to be together in this PR

I think this PR should be merged and get into beta.25.

@filipesilva
Member

@tsm91 that lazy loaded scripts/styles functionality is actually a different thing than lazy loaded routes. See #3401 for details about them.

@tsm91
tsm91 commented Jan 10, 2017

@filipesilva https://github.com/angular/angular-cli/pull/3402/files i wonder why there is no README.md file in the changes. This feature is not even documented.

@clydin clydin add tests
0690808
@clydin clydin changed the title from [WIP]feat: allow output hashing to be configured to feat(build): allow output hashing to be configured Jan 10, 2017
@filipesilva
Member

@tsm91 I didn't add them at the time, which was really a bad choice.

@clydin clydin add docs
90d3e7e
@hansl hansl merged commit b82fe41 into angular:master Jan 10, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@clydin clydin deleted the escorp:output-hash branch Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment