[v3.0] Better esm config file support by lkmill · Pull Request #4123 · rollup/rollup

lkmill · 2021-06-04T15:21:42Z

This PR contains:

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:

#4055

Description

Use a more precise version check to determine ESM support
Load .js config files untranspiled with import() if package.json has "type": "module"

Only checking if major version is above 13 leads to both false negatives and false positives. ESM is supported in ^12.17, ^13.2 and >=14. Thus, version 12.17 has support and 13.0 does not.

Rollup goes against specification when not loading .js config files as ESM despite "type": "module" being set in package.json. This actually causes imports of .cjs files to fail with:

Error: 'default' is not exported by ../../babel.config.cjs, imported by ../../rollup.config.js

Changing the extension of the Rollup file to .mjs resolves the issue.

Although this PR is a breaking change, it makes Rollup handle ESM config files the same way Node handles them natively. It will only make builds with "type": "module" and a .js Rollup config file with invalid ESM syntax fail. Moreover, projects with "type": "module" will already have spent time converting .js files (including config) to ESM or changing their extension to .cjs. If the Rollup config is not valid ESM, it's most likely because they forgot to check it or made a mistake.

github-actions · 2021-06-04T15:23:04Z

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install lohfu/rollup#better-esm-config-file-support

or load it into the REPL:
https://rollupjs.org/repl/?pr=4123

lukastaegert

I am not completely sure about this one, even for a major release. Maybe we actually make this much simpler and say we always use dynamic import(). This would work for CommonJS just as well as ESM, and then we just always add the interop check, i.e. roughly

const configFileExport = supportsNativeESM()
  ? getDefaultFromCjs(await import(pathToFileURL(fileName).href)).default)
  : extension === '.cjs'
  ? getDefaultFromCjs(require(fileName))
  : await getDefaultFromTranspiledConfigFile(fileName, commandOptions.silent)

That way, we do not rely on an additional package but directly use Node's algorithm to detect ESM/CommonJS. Thoughts?

lukastaegert · 2021-06-11T06:11:18Z

+	const major = parseInt(version[1], 10);
+	const minor = parseInt(version[2], 10);
+
+	return major >= 14 || (major === 13 && minor >= 2) || (major === 12 && minor >= 17);


That is definitely a reasonable improvement that I would merge even without a major release.

codecov · 2022-07-06T07:42:04Z

Codecov Report

Merging #4123 (9cbfb21) into release-3.0.0 (48ce34d) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@                Coverage Diff                @@
##           release-3.0.0    #4123      +/-   ##
=================================================
- Coverage          98.87%   98.87%   -0.01%     
=================================================
  Files                211      211              
  Lines               7391     7388       -3     
  Branches            2105     2103       -2     
=================================================
- Hits                7308     7305       -3     
  Misses                27       27              
  Partials              56       56

Impacted Files	Coverage Δ
cli/run/loadConfigFile.ts	`94.00% <ø> (-0.34%)`	⬇️

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 48ce34d...9cbfb21. Read the comment docs.

lukastaegert · 2022-07-06T08:08:59Z

As we are now finally starting work on a new major version, I revisited this PR, rebased it onto the release branch and applied some simplifications:

The version check will no longer be necessary as Node 14 will be the new baseline
I reduced the cases to "transpile or use import()" where the logic is
- We transpile (to CommonJS) if config plugins are used (because then we MUST transpile) or if the extension is .js and the type is not explicitly "module". The reason we transpile the latter is because there is a huge number of "legacy" code bases that use an ES module for Rollup's config file without setting "type": "module", and I would want to avoid the churn. We could consider warning in that case, though, but I am undecided. Thoughts?
- Otherwise we use import (even for the .cjs case for simplicity), checking is there is an __esModule flag on the default export (I feel that check should not cause too many issues for ESM files)

lukastaegert · 2022-07-06T08:10:21Z

Also, the relevant documentation will need to be reworked to better reflect the new logic.

…eprecated features (#4552) * Remove all active deprecations * Make all inactive deprecations active * Try to make test more stable * Update CLI help screen

* Initial new hashing idea * Simplify external import path generation 197 broken tests left * Use correct file names in chunk info 197 broken tests left * Implement first draft for hashing algorithm 189 broken tests left * Remove active deprecations this.emitAsset this.emitChunk this.getAssetFileName this.getChunkFileName import.meta.ROLLUP_ASSET_URL_ import.meta.ROLLUP_CHUNK_URL_ * Reduce render parameters * Always scan all chunks for hashes * Fix asset emission and remaining tests * Reintroduce augmentChunkHash and get OutputChunk by converting RenderedChunk * Provide chunk graph in renderChunk * Handle hash collisions * Remove deprecated hacky asset emission * Allow to configure hash sizes per file * Update documentation * Extend tests * Minor improvements * Improve documentation about hashing * Replace hash in sourcemap file * Provide ChunkInfo in banner/footer/intro/outro * Extract hashing logic * Clean up hashing logic * Add ExternalChunk wrapper * Store inputBase on Chunk * Store snippets on Chunk * Align chunk interfaces * Reduce this. property access * Move dynamicImportFunction warning to options normalization * Restructure rendering logic * Do not run on Node 10 * Update documentation * Try to fix Windows tests * Improve coverage * Remove graph background colors

* fix: run output plugins last * Add test Co-authored-by: Lukas Taegert-Atkinson <[email protected]>

* Convert scripts to ESM, update dependencies * Fix lint issue

lukastaegert · 2022-07-10T12:46:20Z

I updated the documentation now, mostly removing some sections and recommending to use rollup.config.mjs or add type: "module" in your package.json file. From my side this is good to go.

lukastaegert · 2022-07-30T13:00:48Z