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

Use id of last module in chunk as name base for auto-generated chunks #3025

Merged

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Aug 5, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

This will avoid the generic chunk name for auto-generated chunks by using the last module in the execution order of a chunk as base for the [name] part of the file name. The rationale is that since dependencies are always sorted first in the execution order, the last module in a chunk is what was originally imported by at least part of the chunks that depend on the auto-generated chunk.

To get something close to the old naming scheme, one could use output.chunkFileNames: 'chunk-[hash].js'

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Aug 5, 2019

@jakearchibald I assume this would also solve the original issue of #2793 even though it is not configurable?

@codecov
Copy link

@codecov codecov bot commented Aug 5, 2019

Codecov Report

Merging #3025 into unified-file-emission-api will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@                      Coverage Diff                      @@
##           unified-file-emission-api    #3025      +/-   ##
=============================================================
+ Coverage                      88.63%   88.65%   +0.01%     
=============================================================
  Files                            165      165              
  Lines                           5693     5694       +1     
  Branches                        1738     1737       -1     
=============================================================
+ Hits                            5046     5048       +2     
  Misses                           388      388              
+ Partials                         259      258       -1
Impacted Files Coverage Δ
cli/run/timings.ts 0% <ø> (ø)
cli/run/index.ts 86.79% <ø> (ø)
cli/run/batchWarnings.ts 25.61% <ø> (ø)
cli/index.ts 57.14% <ø> (ø)
cli/sourceMappingUrl.ts 100% <ø> (ø)
cli/run/loadConfigFile.ts 83.33% <ø> (ø)
src/watch/index.ts 81.35% <ø> (ø) ⬆️
cli/run/resetScreen.ts 62.5% <ø> (ø)
cli/logging.ts 78.94% <ø> (ø)
cli/run/watch.ts 41.97% <100%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4f44c0...dd0ed65. Read the comment docs.

* Use new FileEmitter for basic cases around assets. TODO: Replace asset
hooks completely, create tests for assets using the new hooks.

* Migrate assets to new file emitter

* Remove assetsById from Graph

* Implement emitFile for assets

* Internally use EmittedFile in the file emitter

* Deprecate emitAsset and ROLLUP_ASSET_URL

* Deprecate getAssetFileName

* Merge chunk emission into unified API and deprecated previous API

* Allow emitting files with fixed names

* Support unnamed assets

* Improve chunk name assignment

* Initial support for chunk file names

* Allow specifying explicit file names for emitted chunks

* Fix some TODOs

* Test ids remain stable when the transform hook is cached and make test
more stable

* Refine error handling

* Test some more errors

* Refine file emission

* Refactor file emitter to have a single code path for asset finalization

* Deduplicated emitted assets without a specific file name

* Only use the alias as name for a manual chunk if the chunk is not facade
for an entry point

* Generate separate facades for duplicate named user-defined entry points

* Always create facades for explicit file names

* Test edge cases

* Test and refactor handling of dynamic relative imports

* Use async-await in generate function, remove error condition

* Improve and test pattern validation

* Test file emitter edge cases

* Improve plugin error handling

* Add documentation
@tivac
Copy link
Contributor

@tivac tivac commented Aug 5, 2019

This will be huge. I was just pondering if this was achievable via a plugin using some real goofy code but I like this version way better 😄

@jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Aug 5, 2019

@lukastaegert happy to give it a spin. Is it in a testable state now?

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Aug 5, 2019

Definitely testable.

@jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Aug 5, 2019

Yeah this is worlds better. GoogleChromeLabs/proxx#493

I still get a few chunk names named index, because their module is process-animation-whatever-blah/index.js. Maybe if multiple modules have the same file name, dedupe using the parent directory name?

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Aug 5, 2019

Maybe, but I'm not really sure this is always a good idea. Also somewhat difficult from where the assignment happens as it does not know the other names.

@jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Aug 5, 2019

Maybe index is common enough to special-case?

lukastaegert and others added 2 commits Aug 5, 2019
* Switch to a code-splitting build and update dependencies

* Fix case where default exports were not properly deconflicted against
chunk names
@lukastaegert lukastaegert force-pushed the better-generated-chunk-names branch from 6e6dec9 to dd0ed65 Aug 5, 2019
@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Aug 5, 2019

I think I will start with the current approach as it is very simple implementation-wise and can be rolled out immediately and we can think about further improvements in another task. Other approaches will both require more implementation work and have more edge-cases that need to be considererd and I'm not sure about some of them. Maybe it is actually possible to do it based on conflicts and then maybe create names such as directory_index.

@lukastaegert lukastaegert merged commit af19c54 into unified-file-emission-api Aug 5, 2019
9 checks passed
@lukastaegert lukastaegert deleted the better-generated-chunk-names branch Aug 5, 2019
@lukastaegert lukastaegert restored the better-generated-chunk-names branch Aug 5, 2019
@lukastaegert lukastaegert deleted the better-generated-chunk-names branch Aug 5, 2019
@lukastaegert lukastaegert mentioned this pull request Aug 5, 2019
9 tasks
lukastaegert added a commit that referenced this issue Aug 5, 2019
…#3025)

* Use new FileEmitter for basic cases around assets. TODO: Replace asset
hooks completely, create tests for assets using the new hooks.

* Migrate assets to new file emitter

* Remove assetsById from Graph

* Implement emitFile for assets

* Internally use EmittedFile in the file emitter

* Deprecate emitAsset and ROLLUP_ASSET_URL

* Deprecate getAssetFileName

* Merge chunk emission into unified API and deprecated previous API

* Allow emitting files with fixed names

* Support unnamed assets

* Improve chunk name assignment

* Initial support for chunk file names

* Allow specifying explicit file names for emitted chunks

* Fix some TODOs

* Test ids remain stable when the transform hook is cached and make test
more stable

* Refine error handling

* Test some more errors

* Refine file emission

* Refactor file emitter to have a single code path for asset finalization

* Deduplicated emitted assets without a specific file name

* Only use the alias as name for a manual chunk if the chunk is not facade
for an entry point

* Generate separate facades for duplicate named user-defined entry points

* Always create facades for explicit file names

* Test edge cases

* Test and refactor handling of dynamic relative imports

* Use async-await in generate function, remove error condition

* Improve and test pattern validation

* Test file emitter edge cases

* Improve plugin error handling

* Add documentation

* Use id of last module in chunk as name base for auto-generated chunks
@philipwalton
Copy link

@philipwalton philipwalton commented Aug 19, 2019

FWIW, looks like this change broke a few things for a few people. I outlined my issue in this comment: #2839.

@isidrok
Copy link
Contributor

@isidrok isidrok commented Aug 24, 2019

FWIW I did a POC with some small plugins to tweak dynamic import names using the file emission API and relying on chunk deduping by ids. Haven't tested them in a real environment and need some externals and error handling but I think they may be good enough to solve most chunk naming issues.

https://gist.github.com/isidrok/d42cb4e71e9c54fe0260ffe300125ef1

@shellscape
Copy link
Contributor

@shellscape shellscape commented Aug 24, 2019

@isidrok that is really interesting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants