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

Proposal: Module Loader API #8327

Open
kitsonk opened this issue Nov 10, 2020 · 21 comments
Open

Proposal: Module Loader API #8327

kitsonk opened this issue Nov 10, 2020 · 21 comments
Labels
cli related to cli/ dir needs discussion this topic needs further discussion to determine what action to take suggestion suggestions for new features (yet to be agreed)

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Nov 10, 2020

With ES Modules, it is effectively impossible to do "hot module reloading" of the modules, as ES Module specification is fairly strict that once the module is resolve and its two pass instantiation is done, that is the end of the road for the module and cannot be replaced.

When you are mocking/testing/etc. though, it is potentially valuable to make changes to a module and "reload" it via dynamic import. In Node.js, they have solved this problem with an experimental loader API. For an example of how this gets integrated to provide a mechanism for module replacements of imports that are still modules, quibble provides ESM support. It basically creates a unique specifier for every time the module is requested to get around the challenge of ES modules being static once resolved.

This would also support solving things like #1739, which we have long wanted to do.

@chrisabrams
Copy link

@kitsonk did you ever come across a solution for readTextFile invalidation?

@gherciu

This comment has been minimized.

@bradthomasbrown
Copy link

bradthomasbrown commented Nov 2, 2024

image
It seems the Deno team not only closes the issues, doesn't offer much in terms of news or progress, but is also minimizing when this is pointed out.
That's incredibly disappointing, so far I have found Deno highly preferable as a very lightweight and powerful scripting/programming engine.

Could the Deno team perhaps confirm explicitly that this is something they will not pursue, so this issue can be closed and those watching it and related issues can get closure?

@petamoriken
Copy link
Contributor

This issue is being discussed on the TC39 side.
https://github.com/tc39/proposal-esm-phase-imports
https://github.com/tc39/proposal-compartments

The following slide from the 8 October 2024 TC39 meeting is helpful.
https://docs.google.com/presentation/d/1HF4COMfypVzftilhOIlGxz9Fn1iaiFdwYDZwzqQt1gs/edit?usp=sharing

@gherciu
Copy link

gherciu commented Nov 3, 2024

The position of Deno team is a bit strange:

  • It copies most of the Node API, but says they follow TC39, even tho Node has such module loader api
  • They add global Deno.something apis which are not TC39 compliant and never mentioned there but for this feature they do not do that
  • They make claims that everything node related is supported using some node layer, and you trust that and when you port your project you find this issue which has 4 years and are totally disappointed because you now are a victim of marketing, and you spent your time to port your project to deno even tho it doesn't fully work

So we follow or not node standards, or we follow only TC39
This is a circle that just slows down the Deno adoption, we need a strong position on what is the path, because if you try to sit on 2 chairs nothing good happens in the end

@petamoriken
Copy link
Contributor

petamoriken commented Nov 3, 2024

  • They make claims that everything node related is supported using some node layer, and you trust that and when you port your project you find this issue which has 4 years and are totally disappointed because you now are a victim of marketing, and you spent your time to port your project to deno even tho it doesn't fully work

I think that statement is incorrect. it wasn't neglected for 4 years, just that the experimental (unstable) API of Node.js is not implemented.
https://github.com/denoland/deno/tree/main/ext/node/polyfills#experimental

I understand the frustration of not getting the features you want, but if you want the features you want included, you can contribute in some way by discussing their usefulness for compatibility or actually implementing them to Deno 🙂

@petamoriken
Copy link
Contributor

For example, it's quite useful to report that the widely used npm package does not work properly due to the lack of implementation of the Node.js experimental Loader API.

@gherciu
Copy link

gherciu commented Nov 3, 2024

We know this is a experimental feature
But people come to this because require.cache doesn't work with import in Deno, and that would be an alternative to at least have a custom loader and somehow remove the import cache which is impossible at the moment in Deno.

If you check most of the issues above mentioned, are related to hmr, require.cache, dynamic imports and peoples get redirected to this issue and the ones with the actual bugs are closed by Deno team by mentioning this one for these 4 years, here is the actual issue, if we had the working require.cache with import then no one would even access this 4 years old issue till these days

example: #25742

@petamoriken
Copy link
Contributor

petamoriken commented Nov 3, 2024

Please calm down. If you want to fix a problem with require.cache, you just provide a repository that shows that you can run it in Node.js but not in Deno.

Sorry but without a full reproduction I can't really say what's going wrong. If you have a repo that can be tested then I can take a look.

#25742 (comment)

(This discussion is obviously not directly related to experimental Module Loader API in Node.js. It might be better to hide their comments as unrelated discussions. It is better to report this problem in a new issue)

@gherciu
Copy link

gherciu commented Nov 3, 2024

The issue which was closed has a clear description with reproduction steps, please re-read it

How is that related to this issue
In most of these issues you are redirected by Deno team to this issue, please re-read the issue and you'll see
Even if yes some of them are not related, but they give this as alternative , even if not implemented even the alternative

@gherciu
Copy link

gherciu commented Nov 3, 2024

Screenshot, just in case
And overall closing and redirecting to some 4yr issue is a bad behavior, not even mentioning minimizing a totally fair comment above (Where I just asked for news)
Screenshot 2024-11-03 at 17 59 58

@petamoriken
Copy link
Contributor

Check the timeline. The issue was closed on September 20; a Deno team member requested a repository to reproduce your issue on September 21.

Again, could you please provide a repository to reproduce the issue?

@gherciu
Copy link

gherciu commented Nov 3, 2024

The reproduction was requested for another thing here #25742 (comment)
Which is another bug in Deno,
And yes the issue was closed and then a reproduction was requested which is not how things should work btw
Should be the opposite, firstly we discuss and then close if there is no such bug btw
And was closed not because of missing reproduction.... the original one they were able to reproduce btw

Thx

@bradthomasbrown
Copy link

you can contribute in some way by discussing their usefulness for compatibility or actually implementing them to Deno 🙂

The usefulness was discussed, in many issues. Those discussions were closed and redirected here. The only discussion here was minimized (read: censored).
My impression is the exact opposite: no, nobody can contribute in some way by discussion and the Deno team has showed precisely why (no response + censoring).

I had plans to implement something that could help but which would ultimately be a workaround, but seeing that discussion is censored by the Deno team if it is critical, but fair, to the Deno team drives me to not implement the feature in Deno.

The emoji usage comes off as passive aggressive, at least to me.

I also personally don't understand what Node.js or TC39 have to do with considering the functionality for Deno. It should not be required, for some feature, for it to be implemented somewhere else before it is considered for Deno. Otherwise, why use Deno?

@bartlomieju
Copy link
Member

Could you folks list which packages you are trying to run that is blocked by not having access to loaders in Deno? Most recently we saw an issue with Playwright that uses TypeScript loader (that shouldn't really be necessary in Deno, but alas), but I'd like to gauge for a wider range of use cases before we undertake this.

Keep in mind that solving this issue will most likely result in worse performance for loading modules as Deno would have to check for registered loaders and potentially hopping multiple times between Rust and JavaScript code to actually load a file before handing it off to V8 for actual execution.

@gherciu
Copy link

gherciu commented Nov 5, 2024

In my case, we build a new package which will need to load user code, and this code may change at runtime, and since require.cache doesn't work correctly in Deno with import (you can't use even require inside a file that uses import) we can't refresh the cache at runtime (Which is a requirement, and we can't stop the deno process, because it will close server connections), and Deno team redirected me to this alternative Module.Loader
In deno you can load a different version using ?query=param but it just loads the first level but the children import again it takes from cache

This package will be used by a big tech company (I can't say the name due to the contract)

For example in Bun the require.cache also shows the modules loaded with import (and you can use these both in same file and no Module related error is thrown) and that is a option we think now, maybe to move to Bun, if in Deno there is no way to achieve it, but Bun has at the moment a small issue to fix, it doesn't return the children imports of a module and is hard to figure out which children's also needs to be deleted from cache and we wait for that

@petamoriken
Copy link
Contributor

Since caching is defined in the language specification when using ES Modules (Cyclic Module Records), this seems to be the only way to accomplish this in CommonJS (Abstract Module Records).

For example in Bun the require.cache also shows the modules loaded with import (and you can use these both in same file and no Module related error is thrown) and that is a option we think now

My understanding is that Bun's implementation deviates from JavaScript...


By the way, this issue could be solved by implementing require(esm) feature in Node.js v23 to Deno. That feature seems to work without a flag, but it is still experimental.

I confirmed that the require.cache contains the value of the module:

use require(esm) in Node.js v23

@gherciu
Copy link

gherciu commented Nov 5, 2024

At the moment in Deno, import and require just live separate lives.
You can't interact between these 2.

In node you at least can compile by yourself .ts to .js and transform all imports to require and you have at the runtime only one way of importing things and you can remove the cache. In Bun this is partially implemented even tho they have a bug at the moment. But they have an alternative to that which is Bun.plugin you may use it as a loader which is an alternative to Module.loader

But in Deno there is no compile step and everything stays as import or require and you just can't uncache something that used import, like literally there is no way. You can't use require.cache with import and the Loader alternative proposed is not even implemented.

@bartlomieju
Copy link
Member

At the moment in Deno, import and require just live separate lives.
You can't interact between these 2.

You can interact between them just like in Node.js.

In node you at least can compile by yourself .ts to .js and transform all imports to require and you have at the runtime only one way of importing things and you can remove the cache. In Bun this is partially implemented even tho they have a bug at the moment. But they have an alternative to that which is Bun.plugin you may use it as a loader which is an alternative to Module.loader

You can compile these files yourself in Deno as well and you can require them as well. You can only remove from cache if you call require(), you can't remove from cache if you import() something, neither in Node, nor Deno. I can't speak about Bun, because I haven't used that API.

Let's see a short example:

// main.cjs
(async () => {
    const fs = require("fs");
    fs.writeFileSync("./import.mjs", "export default { foo: 'bar' };");
    const foo = await import("./import.mjs");
    console.log(foo);

    console.log(require.cache);
    Object.keys(require.cache).forEach(key => delete require.cache[key]);
    console.log(require.cache);
    
    fs.writeFileSync("./import.mjs", "export default { foo: 'fizz' };");
    const foo1 = await import("./import.mjs");
    console.log(foo1);
})();
$ node main.cjs
[Module: null prototype] { default: { foo: 'bar' } }
[Object: null prototype] {
  '/Users/ib/dev/scratch_node_import/main.cjs': {
    id: '.',
    path: '/Users/ib/dev/scratch_node_import',
    exports: {},
    filename: '/Users/ib/dev/scratch_node_import/main.cjs',
    loaded: true,
    children: [],
    paths: [
      '/Users/ib/dev/scratch_node_import/node_modules',
      '/Users/ib/dev/node_modules',
      '/Users/ib/node_modules',
      '/Users/node_modules',
      '/node_modules'
    ],
    [Symbol(kIsMainSymbol)]: true,
    [Symbol(kIsCachedByESMLoader)]: false,
    [Symbol(kIsExecuting)]: false
  }
}
[Object: null prototype] {}
[Module: null prototype] { default: { foo: 'bar' } }
$ deno -RW main.cjs
[Module: null prototype] { default: { foo: "bar" } }
[Object: null prototype] {
  "/Users/ib/dev/scratch_node_import/main.cjs": Module {
    id: ".",
    path: "/Users/ib/dev/scratch_node_import",
    exports: {},
    filename: "/Users/ib/dev/scratch_node_import/main.cjs",
    loaded: true,
    parent: null,
    children: [],
    paths: [
      "/Users/ib/dev/scratch_node_import/node_modules",
      "/Users/ib/dev/node_modules",
      "/Users/ib/node_modules",
      "/Users/node_modules",
      "/node_modules"
    ]
  }
}
[Object: null prototype] {}
[Module: null prototype] { default: { foo: "bar" } }

But in Deno there is no compile step and everything stays as import or require and you just can't uncache something that used import, like literally there is no way. You can't use require.cache with import and the Loader alternative proposed is not even implemented.

You can have a dedicated compilation/transpilation step like a lot of frameworks do (Vite, Next.js, etc). It's the transpilation output that determines if file uses require() (ie. CommonJS format) or import() (ie. ES modules format).

Altering require.cache doesn't have impact on import in Node either - as per the ES spec, once an ES module is loaded it can't be unloaded.

While I can see why loaders might be a useful feature, there's a whole ecosystem that worked without loaders by handling transpilation it self. It is expected that it will work exactly the same in Deno as it does in Node.js.

If you have some example code that works in Node, but has wrong behavior in Deno I'll be glad to take a look and fix the bug.

By the way, this issue could be solved by implementing require(esm) feature in Node.js v23 to Deno. That feature seems to work nodejs/node#55085, but it is still experimental.

Deno supports require(ESM) as of Deno v2.0.

@petamoriken
Copy link
Contributor

Deno supports require(ESM) as of Deno v2.0.

I didn't know that. Thanks!

@gherciu
Copy link

gherciu commented Nov 5, 2024

You can compile these files yourself in Deno as well and you can require them as well.

I mean, why then use Deno? If in the end I can run compiled files with Node, that doesn't make sense
The whole point of not transpiling files and running TS as is, just gone then

So the idea is just to move back to node and do the compilation to commonJS, that's what I understand

And that's the problem, we apply a Node mindset to Deno

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir needs discussion this topic needs further discussion to determine what action to take suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

6 participants