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
Fully resolve absolute imports #79
Fully resolve absolute imports #79
Conversation
Array.from; | ||
Math.clz32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clz32
is different from .from
because it specifies package.json#exports
. This test (correctly) shows that it's ignored, because Node.js doesn't consider it when loading absolute paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctly shows that what is ignored? I’m a bit confused here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I’m pretty sure package.json exports aren’t considered by node unless you’re using a bare identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full output is
import "<CWD>/array.from/auto.js";
import "<CWD>/math.clz32/auto.js";
Array.from;
Math.clz32;
I added clz32
to the test to check that it doesn't inject import "<CWD>/math.clz32/auto"
(because of the exports
map which includes "/auto"
and not "/auto.js"
), since exports
should be ignored when using absolute paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh ok. why is this preferable over bare identifiers? Absolute paths aren’t portable across machines - doesn’t this increase the likelihood they’ll end up in a published package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can indeed rely on that, and using peerDeps, you should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You really can't, If I were to tell next.js to transpile @material-ui/core
with the core-js
version it has access to webpack will throw because it can't find the files in core-js@2
.
└── node_modules
├── next.js
├── core-js@3.9
└── @material-ui/core
└── node_modules
└── core-js@2
For your own project sure, you can do it with peer dependencies but that would force all projects using Next.js, create-react-app
, and storybook
etc. to explicitly depend on @babel/runtime
and/or core-js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s because core-js should be a peer dep of material-ui. I’d file a bug there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@material-ui/core
doesn't actually depend on core-js
, I used that name as an example because people usually don't do well with abstracts. If it was a peer dependency then it would be even worse because the root couldn't satisfy both core-js@3.9
and core-js@2
at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the point - if it peer depends on core-js 2, even implicitly, then it's incompatible with your project and you simply can't use it.
2e52cf9
to
b96e8e8
Compare
b63f4c9
to
3e531b3
Compare
b96e8e8
to
072d8eb
Compare
try { | ||
let pkg; | ||
if (nativeRequireResolve) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR: we need to update nativeRequireResolve
to use cheap semver so versions like 8.10
does not fall to requireResolve
.
072d8eb
to
aeca57f
Compare
Node.js doesn't resolve absolute import specifiers, so we have to provide the full path (with the extension, and potentially with the resolved internal file).
Ref: babel/babel#12827