-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(runtime): align import.meta.resolve with node.js's implementation #5827
Conversation
packages/bun-types/globals.d.ts
Outdated
|
||
/** | ||
* Resolve a module ID the same as if you imported it | ||
* | ||
* The `parent` argument is optional, and defaults to the current module's path. | ||
* | ||
* Will throw if the module does not exist. |
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.
Erm... it should not throw an error if you do:
import.meta.resolve('./this-file-dose-not-exist.md') // or
import.meta.resolve('./this-dir-dose-not-exist')
That is not how Deno, browser or NodeJS works right now.
if the module can't be resolved by looking at some import-map or some package.json
then it should just return the relative string.
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.
this documentation is for resolveSync
not resolve
auto specifier = specifierValue.toWTFString(globalObject); | ||
RETURN_IF_EXCEPTION(scope, JSC::JSValue::encode(JSC::JSValue {})); | ||
|
||
// Node.js allows a second argument for parent |
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.
only behind a flag. it's likely that NodeJS will not support a 2nd argument for import.meta.resolve
as they want to align with Deno and Browser that dose not support a 2nd argument.
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.
I think as of v20.11.1, import.meta.resolve
supports second argument (also for what is worth, if it is not supported, tooling support would be hell considering a module cannot resolve on behalf of another)
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.
it is still behind a flag: --experimental-import-meta-resolve
since we do not have node's feature flags, i think this is ok to just have, but it can just not be present in the typescript definitions
❌ @paperdave 3 files with test failures on bun-darwin-aarch64: |
❌ @paperdave 3 files with test failures on linux-x64: |
❌ @paperdave 3 files with test failures on linux-x64-baseline: |
❌ @paperdave 11 files with test failures on bun-windows-x86_64-haswell
|
❌ @paperdave 5 files with test failures on bun-darwin-x64: |
* do not lookup cwd in which * fix webkit submodule * fix compilation on linux * feedback
* fix canceled onFileRead * report continue errors and fix closing * also fix pipe writer * avoid possible memory leaks * Propagate errors in open --------- Co-authored-by: Jarred Sumner <[email protected]>
posix test failrues test/bundler/bundler_compile.test.ts window test failure test\js\bun\resolve\import-meta-resolve.test.mjs |
test\bundler\bundler_compile.test.ts panics on windows |
What does this PR do?
Closes #2472
How did you verify your code works?