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

Rollup should fail on extensionless imports in a type: module package #4301

Open
Tracked by #2 ...
elado opened this issue Dec 14, 2021 · 12 comments · May be fixed by #4303
Open
Tracked by #2 ...

Rollup should fail on extensionless imports in a type: module package #4301

elado opened this issue Dec 14, 2021 · 12 comments · May be fixed by #4303

Comments

@elado
Copy link

elado commented Dec 14, 2021

Rollup Version

v2.61.1

Operating System (or Browser)

macOS

Node Version (if applicable)

v16.13.1

Link To Reproduction

https://replit.com/@elado/rollup-repro#src/index.js

Expected Behaviour

Rollup should fail a build when package has "type": "module" in package.json and sources import extensionless files.

This becomes necessary in certain scenarios, like externals and preserveModules, where the output file keeps selective imports that will fail at runtime.

Example 1:

// preserveModules: true, plugins: [nodeResolve(), commonjs()]

// src/index.js
import { local } from './local';

// src/local.js
export const local = 1;

The output file will correctly contain import { local } from './local.js', but this should fail at the rollup build.

Example 2:

// external: (id) => /^lodash/.test(id), plugins: [nodeResolve(), commonjs()]

// src/index.js
import isArray from 'lodash/isArray';

The import is kept as external and the output import remains the same, which results in a runtime error in type: module.

Actual Behaviour

Rollup build succeeds, but runtime fails.

@lukastaegert
Copy link
Member

I would expect this to only be an issue for externals and not for preserveModules as here, all imports are rewritten. Otherwise, this is a complicated topic: rollup itself does not know about Node.js semantics, only @rollup/plugin-node-resolve does. So this would be a feature request for the node-resolve plugin.

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 14, 2021

@lukastaegert I'm currently working on a PR to take care of requiring file extensions as an opt-in, also needed as a pre-cursor for file URLs and query params. should be up shortly.

@kzc
Copy link
Contributor

kzc commented Dec 14, 2021

Otherwise, this is a complicated topic: rollup itself does not know about Node.js semantics, only @rollup/plugin-node-resolve does

That's the official line, but we know that rollup does have some basic knowledge of NodeJS semantics even without any plugins specified in order to append file suffixes:

$ cat module/package.json 
{"type": "module"}
$ cat module/thing1.js 
import "./thing2";
$ cat module/thing2.js 
console.log("thing2.js");

rollup appends the ".js" suffix to import "./thing2"; in order to produce the output:

$ rollup module/thing1.js --silent
console.log("thing2.js");

NodeJS' path.resolve is called here in rollup core:

// `resolve` processes paths from right to left, prepending them until an
// absolute path is created. Absolute importees therefore shortcircuit the
// resolve call and require no special handing on our part.
// See https://nodejs.org/api/path.html#path_path_resolve_paths
return addJsExtensionIfNecessary(
importer ? resolve(dirname(importer), source) : resolve(source),
preserveSymlinks
);
}
function addJsExtensionIfNecessary(file: string, preserveSymlinks: boolean) {
let found = findFile(file, preserveSymlinks);
if (found) return found;
found = findFile(file + '.mjs', preserveSymlinks);
if (found) return found;
found = findFile(file + '.js', preserveSymlinks);
return found;

Rollup core also needs to read and write modules from the filesystem without plugins - something that requires the builtin fs module: https://github.com/rollup/rollup/blob/master/src/utils/fs.ts

I now think that Rollup 3.x should revisit the decision to not fold node-resolve and commonjs functionality into rollup core in order to give a more seamless user experience as other popular bundlers do. Rollup already has to have knowledge of NodeJS - so it might as well completely support it without the hassle of coordinating and specifying compatible versions of rollup, node-resolve and commonjs plugins.

@lukastaegert
Copy link
Member

It has long been discussed if the support for extensionless imports was a mistake, but I do not want to open this again, especially since TypeScript worked hard to shape expectations by enforcing it. But everything else is basically missing, Index imports, and all features that depend on package.json knowledge etc. That may be what most users want, but the separation has enabled quite a few niche usages for Rollup that set it apart. Also it allows others to completely customise the experience. There are quite a few tools that build on Rollup these days that give users the tailored experience, especially looking at Vite here.

@kzc
Copy link
Contributor

kzc commented Dec 14, 2021

I see your point - there's a lot of legacy usage out there. But what if the node-resolve and commonjs plugins were to just ship with rollup 3.x core and could be used with an opt-in flag? That alone would make it easier to use in the vast majority of user scenarios - no versioning, and no need to load or import them. I'd argue the flag should be an opt-out style option in rollup 3.x, but that's less likely to gain consensus.

@dnalborczyk
Copy link
Contributor

@lukastaegert

especially since TypeScript worked hard to shape expectations by enforcing it

curious, what do you mean by that?

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 14, 2021

I see your point - there's a lot of legacy usage out there. But what if the node-resolve and commonjs plugins were to just ship with rollup 3.x core and could be used with an opt-in flag? That alone would make it easier to use in the vast majority of user scenarios - no versioning, and no need to load or import them. I'd argue the flag should be an opt-out style option in rollup 3.x, but that's less likely to gain consensus.

@kzc not sure if node-resolve and commonjs should go into core. that said, I think you (or someone else) mentioned somewhere that node-resolve and commonjs should potentially be one single plugin, to that I think I would agree, because commonjs as far as I know is only being used by node.js and so is the node.js resolution algorithm (which currently only works for commonjs as well).

I hope that the node.js resolution flag for esm will never be activated by default and subsequently be removed, as it only causes problems and makes everything more complex.

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 14, 2021

@lukastaegert I think Typescript had other (technical) reasons to not enforce extensions. Deno opted in by requiring them as well. I can't find any references of why Typescript decided not to require extensions.

(the esm version of) rollup itself only runs in the browser and in node.js because it's being bundled, otherwise it would be not possible to run rollup "unbundled", as the file extensions are missing.

to achieve running node.js esm, one has to use explicit file extensions with Typescript files as well.

@kzc
Copy link
Contributor

kzc commented Dec 14, 2021

I'm not suggesting to change the rollup core architecture. Just proposing to ship the plugins most users need with rollup core - node-resolve, commonjs and possibly json - and provide an opt-in shorthand option to enable those plugins with default options. It would only require a very small change to the rollup CLI. If users wish to use their traditional rollup config files, or command line options - it would not break backwards compatibility.

@dnalborczyk
Copy link
Contributor

I'm not suggesting to change the rollup core architecture.

yeah, I figured. with core I meant rollup itself, not the implementation.

@dnalborczyk dnalborczyk linked a pull request Dec 15, 2021 that will close this issue
13 tasks
@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 15, 2021

one thing I also noticed:

// index.js
import { foo } from "./foo";

console.log(foo);
// foo.js
export const foo = 1;

with preserveModules: true becomes:

// index.js
import { foo } from './foo.js';  //  <--- extension included :P

console.log(foo);
// foo.js
const foo = 1;

export { foo };

if this is not a bug but intentional, I would argue this to be another reason for requiring file extensions. 😉

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 15, 2021

@kzc configuring rollup with commonjs and node-resolve can definitely be a PITA. I have tried so many times (for node.js server code and lambdas etc.), that I have usually always switched to webpack (if I wanted to have the dependencies included, which, unfortunately, are mostly still commonjs).

I also get @lukastaegert reasoning to not have those included in core. for one, rollup started out as an esm-only bundler, expecting esm-only as input. as it looks for commonjs, the clock started ticking for it to slowly fade out of existence. granted, it might take years.

speaking of which, there is more module proposals on the horizon. should those in turn be included as well? 🤔
css modules: https://github.com/WICG/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md
html modules: https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/HTMLModules/designDoc.md
wasm modules ...

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

Successfully merging a pull request may close this issue.

4 participants