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

Unified file emission api #2999

Merged
merged 30 commits into from Aug 5, 2019
Merged

Unified file emission api #2999

merged 30 commits into from Aug 5, 2019

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jul 16, 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:
Resolves #2938

Description

This PR will implement a new unified file emission API for plugins. The API will consist of

  • A new this.emitFile context function to emit both chunks and assets that supports specifying explicit file names
  • A new this.getFileName plugin context function to get the file name of emitted assets and chunks
  • A new import.meta.ROLLUP_FILE_URL_<xxx> way of referencing emitted files from code
  • A slight modification to the resolveFileUrl hook to unify how file reference ids are specified for assets and chunks

this.emitFile(emittedFile: EmittedFile) => string

With

type EmittedFile = {
  type: 'asset';
  name?: string;
  fileName?: string;
  source?: string | Buffer;
} | {
  type: 'chunk';
  name?: string;
  fileName?: string;
  id: string;
}

If fileName is specified, it is used as the file name of the emitted asset or chunk without modification; if there are conflicts with file names of other files, an error is thrown.

Otherwise if name is specified, Rollup tries to use this name together with the corresponding user-defined name pattern for this file. There are not guarantees what the resulting name will be, if several plugins emit the same file with different names, only one file will be emitted. In case of conflict, the file name will be deduplicated.

Otherwise, a default name ('chunk' or 'asset') is used.

Limitations: At the moment, the file names of all assets that already have a source are finalized when generate is called. Also during generate, calling setSource will immediately finalize a file name. This has the effect that any assets with a specific file name that are emitted later and conflict with any file with a finalized name will throw an error even though the other file may not have had a specific file name specified. The motivation is to make this.getFileName safe to be used throughout generate. This limitation could be eased but I would leave it as it is for the first version. Just using a name is always safe.

Other changes

this.getFileName and import.meta.ROLLUP_FILE_URL_<xxx> work like the corresponding asset and chunk functions used to work; those are marked for deprecation and will throw an error if the strictDeprecations option is used. Otherwise they will show warnings starting with Rollup@2 and be removed in Rollup@3.

resolveFileUrl now provides a referenceId parameter to the hook. In Rollup@3, the assetReferenceId and chunkReferenceId parameters will be removed.

Internally, Rollup will now use the id of the last module in the execution order of any chunk that does not have a facade module as base for the variable name used in CJS and AMD outputs. This should make the output more readable. In a separate PR, I hope to get rid of auto-generated "chunk" chunks and also use the id of the last module as base for the file name.

Open points

The only left open points are

  • Add documentation
@codecov
Copy link

@codecov codecov bot commented Jul 16, 2019

Codecov Report

Merging #2999 into master will increase coverage by 0.76%.
The diff coverage is 99.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2999      +/-   ##
==========================================
+ Coverage   87.86%   88.63%   +0.76%     
==========================================
  Files         166      165       -1     
  Lines        5647     5693      +46     
  Branches     1722     1738      +16     
==========================================
+ Hits         4962     5046      +84     
+ Misses        412      388      -24     
+ Partials      273      259      -14
Impacted Files Coverage Δ
src/utils/defaultPlugin.ts 94% <100%> (+0.12%) ⬆️
src/utils/pluginDriver.ts 87.16% <100%> (+4.49%) ⬆️
src/Graph.ts 93.06% <100%> (-0.31%) ⬇️
src/Module.ts 94.19% <100%> (ø) ⬆️
src/utils/assignChunkIds.ts 100% <100%> (ø) ⬆️
src/rollup/index.ts 93.71% <100%> (+0.06%) ⬆️
src/utils/renderNamePattern.ts 100% <100%> (+40%) ⬆️
src/ModuleLoader.ts 97.38% <100%> (+3.29%) ⬆️
src/utils/error.ts 100% <100%> (ø) ⬆️
src/ast/nodes/MetaProperty.ts 97.95% <100%> (+0.45%) ⬆️
... and 9 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 57dd0b9...e4f44c0. Read the comment docs.

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Jul 24, 2019

@surma @jakearchibald @tivac @manucorporat @wessberg @ryanatkn This PR is nearly ready to be merged and already fully testible (npm i rollup/rollup#unified-file-emission-api). I changed the API slightly from the original propsal to make it more versatile and add fixed names for chunks as well. Feedback is very much welcome while I address the remaining points mentioned in the description.

Once this is released, I would recomment everyone to switch to this API as the previous will eventually be deprecated.

@tivac
Copy link
Contributor

@tivac tivac commented Jul 24, 2019

Thanks for the heads up! I'm hoping to get a modular-css implementation of this API stood up shortly. Dealing with some work deadlines atm so it's taking me longer to get to it than I'd like 😬

@jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Jul 24, 2019

I was able to install the new version and build without changes (it didn't break anything).

I've switched to using the new API in GoogleChrome/devsummit#140.

We're doing some weird gymnastics that that plugin. We're treating CSS as chunks so it can have dependencies. We'll undo all that once a solution to #2823 lands.

The unified file API is great!

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Jul 24, 2019

If you use fixed file names with the new API, maybe you can avoid the gymnastics for now? You would need to do any hashing yourself if you need it but that should not be too difficult. Rollup itself is using hash.js via hash.js/lib/hash/sha/256. Dependencies are still on the roadmap but my development speed is not optimal at the moment.

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Jul 24, 2019

With the new API, getFileName will immediately work for files that have fixed names so you can stick with this API to be able to move to a dependency based API later.

@jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Jul 24, 2019

If you use fixed file names with the new API, maybe you can avoid the gymnastics for now?

I could use fixed files for the HTML, but most of the gymnastics are there for handling dependencies. The CSS is treated as a chunk so that its hash will change if its child assets (images and fonts) also change.

@lukastaegert lukastaegert force-pushed the unified-file-emission-api branch from ba50e05 to 689ba18 Aug 1, 2019
@lukastaegert lukastaegert marked this pull request as ready for review Aug 2, 2019
@lukastaegert lukastaegert changed the title [WIP] Unified file emission api Unified file emission api Aug 2, 2019
@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Aug 2, 2019

This PR is finally complete and will be released in the next days.

@tivac
Copy link
Contributor

@tivac tivac commented Aug 2, 2019

You're awesome! Still haven't had a chance to try it but very excited to stop poking directly into the bundle for things 😅

@lukastaegert lukastaegert merged commit 2443783 into master Aug 5, 2019
9 checks passed
@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Aug 5, 2019

After merging this one I discovered a situation where with the new code-splitting build, Rollup would not always deconflict chunk variable names, which is why I will postpone the release until I get to the bottom of this.

lukastaegert added a commit that referenced this issue Aug 5, 2019
…#3025)

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

* Unified file emission api (#2999)

* 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

* Switch to a code-splitting build and update dependencies (#3020)

* Switch to a code-splitting build and update dependencies

* Fix case where default exports were not properly deconflicted against
chunk names
@wessberg
Copy link
Contributor

@wessberg wessberg commented Sep 5, 2019

Hi @lukastaegert, thanks for your work on this PR.

I'm having some issues, however.

Consider a multi-output Rollup config:

{
  input: "...",
  output: [
    {
      file: "...",
      format: "cjs"
    },
    {
      file: "...",
      format: "esm"
    }
  ],
  // ...
}

And then consider emitting additional assets in the generateBundle hook.

Naively, I would do

this.emitFile({
  type: "asset",
  fileName: "...",
  source: "..."
  }))

But it will lead to the following error: Could not emit file "<filename>" as it conflicts with an already emitted file. (where <filename> could be any file) while calling generateBundle for the second output if emitting an asset with the same name for the 2nd output.

I would assume that these assets would be unique for each output, rather than for the entire compilation?

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Sep 6, 2019

They should be unique per output when emitting them during generateBundle. I will need to look into this, thanks for spotting!

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.

4 participants