Implement synchronous `import.meta.resolve(…)`

lgarron commented 1 year ago

What is the problem this feature would solve?

It is common to write apps and library code in JS that references relative file paths, such as:

Dynamic JS entry files (in particular, for web workers/service workers/shared workers)
WASM
CSS, JSON, arbitrary resources, …

import.meta.resolve(…) provides a standard way to refer to a relative resource, and also supports bare names and absolute paths. It is already supported in all major browsers and deno. It would be valuable for import.meta.resolve(…) to be available in all target environments, including bun.

What is the feature you are proposing to solve the problem?

Implement https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import.meta/resolve

What alternatives have you considered?

As a library author, I can use code that uses new URL(…, import.meta.url).href instead. However:

import.meta.resolve(…) is more ergonomic and conveys clear intentions compared to being a "magic pattern", and
it is not supported by all bundlers, and it's unclear if it ever will be (possibly due to the previous reason).

Jarred-Sumner commented 1 year ago

On the server, module resolution has to be async. We don't know where a module might point to until we've visited other files (like package.json). We may even need to download package.json to find out. That operation should not block the server by default.

Browsers don't have this problem because browser module resolution is very simple.

Bun does implement import.meta.resolve, however it follows what Node.js does which is not the same as what browsers do.

import.meta is one of the few places where runtimes are allowed to have different APIs on the same property.

Per the ECMAScript spec, import.meta is host-defined:

13.3.12.1.1 HostGetImportMetaProperties ( moduleRecord ) The host-defined abstract operation HostGetImportMetaProperties takes argument moduleRecord (a Module Record) and returns a List of Records with fields [[Key]] (a property key) and [[Value]] (an ECMAScript language value). It allows hosts to provide property keys and values for the object returned from import.meta.

Jarred-Sumner commented 1 year ago

That being said, on the bundler side of things Bun can implement the new URL pattern as well as inline the result of import.meta.resolve when statically known (but it does not yet)

lgarron commented 1 year ago

On the server, module resolution has to be async.

Hmm, deno implements it by limiting the scope of resolution (in their case, relative paths as well as bare names using import maps, the former of which at least should be unambiguous in any environment) and node is working on a special sync implementation. Would either of those approaches work?

That being said, on the bundler side of things Bun can implement the new URL pattern as well as inline the result of import.meta.resolve when statically known (but it does not yet)

new URL(…, import.meta.url) (or new URL(…, import.meta.url).href) is certainly very useful, and I'd be glad to see support!

lgarron commented 1 year ago

On the server, module resolution has to be async.

Just wanted to make a note that node has landed synchronous import.meta.resolve(…) for node v20.0.0: https://github.com/nodejs/node/pull/44710#event-8993642914

If/when this goes to stable next week, this means that all the following will support the same synchronous import.meta.resolve(…) functionality:

Chrome (and Edge, Brave, etc.)
Firefox
Safari
node (when using --experimental-import-meta-resolve)
deno

I'd like to be able to support bun in my libraries and don't see an issue with awaiting import.meta.resolve(…) in my own code. This will mostly be needed in places where asynchronous code will already be used, so it shouldn't require a lot of extra work.

However, it seems likely to me that developers and apps in the wider ecosystem will start assuming a synchronous result based on all the other environments. I can also imagine bundlers/minifiers removing the await based on an expected synchronous result, changing bun-compatible code into incompatible code. So this could be subtle compability risk for bun in the future.

lgarron commented 1 year ago

Just wanted to make a note that node has landed synchronous import.meta.resolve(…) for node v20.0.0: nodejs/node#44710 (comment)

If/when this goes to stable next week, this means that all the following will support the same synchronous import.meta.resolve(…) functionality:

FWIW, node did ship with this in v20.0.0 and has released v20.1.0 since, so it looks like it's going to stick. So bun is now unfortunately the lone one out in supporting async import.meta.resolve(…) but not sync. 😭

(I'm still fine with using await for my own code, but as mentioned above I still think this is going to trip up developers whose code works in all other environments the same way.)

guilhermesimoes commented 1 year ago

If I may, I'd like to just comment on the usage of new URL(...) and import.meta.resolve(...).

We at PeacockTV and SkyShowtime need to support all kinds of TVs, set-top boxes and console, some of which were launched more than 10 years ago (PlayStation 4 is one such example). Some of these are based on older versions of Chromium, and lack URL support. Likewise, even if import.meta.resolve(…) is already supported in all major browsers, it most surely is not supported on these devices that I mentioned.

I know we could add polyfills but in order to keep our bundle size small we usually try to rewrite our code before adding one.

What @Jarred-Sumner said:

inline the result of import.meta.resolve when statically known

Is super important and would allow us to deliver smaller builds to this other class of devices.

GeoffreyBooth commented 1 year ago

Hello 👋 I’m part of the team responsible for landing sync import.meta.resolve in Node 20. I found this issue because we’re trying to start a process with WinterCG to standardize import.meta across server-side runtimes such as Node, Deno and Bun. I invite you to join us! See https://github.com/wintercg/proposal-common-minimum-api/pull/49 and https://github.com/wintercg/proposal-common-minimum-api/issues/50.

@guilhermesimoes how do you plan to get import.meta.resolve inlined? Would you be careful to always pass it static strings like import.meta.resolve('foo') and never variables (import.meta.resolve(someStr))? Also hello from Disney, I work on Disney+ and ESPN and related systems and I know your pain 😄

paperdave commented 1 year ago

We already have import.meta.resolveSync, so I think that function can replace import.meta.resolve, though I'm not sure if the sync version handles all async resolution cases. it also doesnt return a url string yet.

lgarron commented 1 year ago

We already have import.meta.resolveSync, so I think that function can replace import.meta.resolve, though I'm not sure if the sync version handles all async resolution cases. it also doesnt return a url string yet.

I think that makes sense when writing code that's only meant for bun. However, import.meta.resolve(…) is defined to be sync in all other environments, which is going to cause issues for code that is not written by someone familiar with bun's diverging behaviour.

If a code author knows about bun and can't await[^1] in a given piece of code, it's theoretically possible to write something like:

import.meta.resolveSync?.(…) ?? import.meta.resolve(…) (not DRY) or
const resolve = import.meta.resolveSync?.bind(import.meta) ?? import.meta.resolve.bind(import.meta); /* … */; resolve(…)

… but I wouldn't want to make a bet that this will be handled correctly by all browsers[^2], bundlers, and transpilers[^3]. I also wouldn't be too enthused to make a pull request to every project that uses import.meta.resolve(…) if I want to use it in bun.

[^1]: For example, top-level await is super tricky for engines and bundlers to get right and may be reasonable to avoid for some projects.

[^2]: Browsers tend to be finicky about the semantics of import stuff, such as causing the entire script to error when import(…) is present in a non-module (i.e. "classic") script, even if it's not used. That theoretically shouldn't be an issue here, but it still makes me super wary of making any assumptions about code transformations even if they "shouldn't" cause issues. The need to call the functions with the correct this is also rather non-obvious, especially if you've only seen built-in functions that don't require it (e.g. console.log).

[^3]: Many of them use exact code matches for functionality like this. (For example, for new URL(…, import.meta.url) and import(…).) And if a particular tool doesn't recognize every variant of such syntax that's needed for workarounds, it may break downstream tools or some target environments.

If there is a need for both sync and async flavors, I'd strongly advocate for import.meta.resolve(…) to be sync import.meta.resolveAsync(…) to be the alternative in this case.

paperdave commented 1 year ago

what i meant was we could use our existing implementation of resolveSync, and rename it to resolve (plus fixing it to return a url string). the mention of the other function was to say most of the work is implemented, and not to say "just use that in your code"

If there is a need for both sync and async flavors, I'd strongly advocate for import.meta.resolve(…) to be sync import.meta.resolveAsync(…) to be the alternative in this case.

This sounds like a good idea to me, most of the work is just swapping the two functions around. There will be a later effort of getting synchronous resolve to work with asynchronous Bun.plugin, though we don't actually support that at all right now.

jimmywarting commented 1 year ago

Also want to point out that it should be possible to resolve anything that dose not resolve into a module.

import.meta.resolve('./' + Math.random())

Deno and browser dose not look up any module when resolving paths. it's more or less a direct approach to new URL('./dist', import.meta.url).toString() with also regards to import-maps..

What is the expected behavior? Why is that the expected behavior?

i did kind of expect import.meta.resolve('./dist) to work more or less similar to: new URL('./dist', import.meta.url).toString() or how the browsers import.meta.resolve() works... but it did not. it throws an error instead

i kind of expect all of the node_modules folder + node core modules to behave more or less import-maps and import.meta.resolve('dust) to throw an TypeError

TypeError: Failed to execute 'resolve' on 'import.meta': Failed to resolve module specifier "dust": Relative references must start with either "/", "./", or "../". Or using import-maps

What do you see instead?

One thing that's happening in Bun is:

<Promise> Error [ERR_MODULE_NOT_FOUND]: Cannot find module <X> imported from <Y>

i think Bun should kind of be creating a import-map.json up front when it install any dependencies

NodeJS just fixed this: https://github.com/nodejs/node/issues/49010

lgarron commented 1 year ago

what i meant was we could use our existing implementation of resolveSync, and rename it to resolve (plus fixing it to return a url string). the mention of the other function was to say most of the work is implemented, and not to say "just use that in your code"

If there is a need for both sync and async flavors, I'd strongly advocate for import.meta.resolve(…) to be sync import.meta.resolveAsync(…) to be the alternative in this case.

This sounds like a good idea to me, most of the work is just swapping the two functions around. There will be a later effort of getting synchronous resolve to work with asynchronous Bun.plugin, though we don't actually support that at all right now.

@paperdave Would you accept a PR for this / if I wanted to tackle this, is there any advice you could give me?

The import.meta.resolve(…) compat chart is sooo close to all green, and I'd love to get it over the finish line. 😄

paperdave commented 1 year ago

we definitely need to align what import.meta.resolve does in node

something im still wondering is if node.js is going to make import.meta.resolve return a string like web browsers and deno do, or keep it as a URL object.

would appreciate a pr for this. most of the code is already there to do this.

jimmywarting commented 1 year ago

something im still wondering is if node.js is going to make import.meta.resolve return a string like web browsers and deno do, or keep it as a URL object.

I hope they keep it as a string to align with what browser are doing.

lgarron commented 1 year ago

we definitely need to align what import.meta.resolve does in node

🤩

something im still wondering is if node.js is going to make import.meta.resolve return a string like web browsers and deno do, or keep it as a URL object.

I hope they keep it as a string to align with what browser are doing.

node has chosen to keep it a URL object. I don't know if the ship has sailed on that, but this seems to be the most pertinent issue: https://github.com/nodejs/node/issues/48994

(FWIW, deno returns a string.)

would appreciate a pr for this. most of the code is already there to do this.

Alright, I'll see if I can get my bun checkout compiling again! (Is there an easy way to build bun in a codespace yet?

lgarron commented 1 year ago

node has chosen to keep it a URL object. I don't know if the ship has sailed on that, but this seems to be the most pertinent issue: nodejs/node#48994

Hmm, interesting. There actually seems to be a mismatch with the documentation: https://nodejs.org/api/esm.html#importmetaresolvespecifier

Returns: <string> The absolute (file:) URL string for the resolved module.

EDIT: filed an issue at https://github.com/nodejs/node/issues/49695

GeoffreyBooth commented 1 year ago

node has chosen to keep it a URL object.

Node's import.meta.resolve returns an URL string, not an URL instance.

lgarron commented 1 year ago

node has chosen to keep it a URL object.

Node's import.meta.resolve returns an URL string, not an URL instance.

Could you describe the difference?

This is what I see in node 20.6:

> echo "console.log(process.versions.node); console.log(import.meta.resolve('./rel'));" > /tmp/test.mjs
> node /tmp/test.mjs
20.6.1
URL {
  href: 'file:///private/tmp/rel',
  origin: 'null',
  protocol: 'file:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: '/private/tmp/rel',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

import.meta.resolve('./rel') instanceof URL also evaluates to true.

jimmywarting commented 1 year ago

node has chosen to keep it a URL object.

Node's import.meta.resolve returns an URL string, not an URL instance.

Could you describe the difference?

This is what I see in node 20.6:

wtf, that is weird, i had to try it out for myself. i installed the latest version 20.6.1 used import.meta.resolve() and i got a string back 👀

then i tried your exact version and then i got back a URL instance. like what gives?!?!

turns out if the file or the directory exist, then you get back a string. if the path dose not exist then it gives you a URL instance.

Sooooo inconsistent

lgarron commented 1 year ago

node has chosen to keep it a URL object.

Node's import.meta.resolve returns an URL string, not an URL instance.

Could you describe the difference? This is what I see in node 20.6:

wtf, that is weird, i had to try it out for myself. i installed the latest version 20.6.1 used import.meta.resolve() and i got a string back 👀

then i tried your exact version and then i got back a URL instance. like what gives?!?!

turns out if the file or the directory exist, then you get back a string. if the path dose not exist then it gives you a URL instance.

Sooooo inconsistent

Oh, woah, nice debugging — definitely looks like it!

> echo "console.log(process.versions.node); console.log(import.meta.resolve('.'));" > /tmp/test.mjs
> node /tmp/test.mjs
20.6.1
file:///private/tmp/

GeoffreyBooth commented 1 year ago

turns out if the file or the directory exist, then you get back a string. if the path dose not exist then it gives you a URL instance.

It’s a bug, fixed in https://github.com/nodejs/node/pull/49698.

paperdave commented 1 year ago

i have to write tests and make sure edge cases are handled but:

(bd is my development bun)

lgarron commented 1 year ago

i have to write tests and make sure edge cases are handled but:

(bd is my development bun)

🥳

Do you plan to submit a PR? I had trouble getting my bun checkout to compile (and it still won't compile on my main computer), but happy to leave it to you if you're planning on a PR in the near future.

oven-sh / bun