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

Node.js Loaders Team Meeting 2022-02-15 #64

Closed
mhdawson opened this issue Feb 11, 2022 · 23 comments · Fixed by #66
Closed

Node.js Loaders Team Meeting 2022-02-15 #64

mhdawson opened this issue Feb 11, 2022 · 23 comments · Fixed by #66
Assignees

Comments

@mhdawson
Copy link
Member

mhdawson commented Feb 11, 2022

Time

UTC Tue 15-Feb-2022 19:00 (07:00 PM):

Timezone Date/Time
US / Pacific Tue 15-Feb-2022 11:00 (11:00 AM)
US / Mountain Tue 15-Feb-2022 12:00 (12:00 PM)
US / Central Tue 15-Feb-2022 13:00 (01:00 PM)
US / Eastern Tue 15-Feb-2022 14:00 (02:00 PM)
EU / Western Tue 15-Feb-2022 19:00 (07:00 PM)
EU / Central Tue 15-Feb-2022 20:00 (08:00 PM)
EU / Eastern Tue 15-Feb-2022 21:00 (09:00 PM)
Moscow Tue 15-Feb-2022 22:00 (10:00 PM)
Chennai Wed 16-Feb-2022 00:30 (12:30 AM)
Hangzhou Wed 16-Feb-2022 03:00 (03:00 AM)
Tokyo Wed 16-Feb-2022 04:00 (04:00 AM)
Sydney Wed 16-Feb-2022 06:00 (06:00 AM)

Or in your local time:

Links

Agenda

Extracted from loaders-agenda labelled issues and pull requests from the nodejs org prior to the meeting.

nodejs/loaders

Invited

  • Loaders team: @nodejs/loaders

Observers/Guests

Notes

The agenda comes from issues labelled with loaders-agenda across all of the repositories in the nodejs org. Please label any additional issues that should be on the agenda before the meeting starts.

Joining the meeting

@mhdawson mhdawson self-assigned this Feb 11, 2022
@guybedford
Copy link

Perhaps we can add the import.meta.resolve discussion to this meeting? Let me know if that might be suitable.

@bmeck
Copy link
Member

bmeck commented Feb 12, 2022 via email

@guybedford
Copy link

Per current discussion in whatwg/html#5572 it seems like having a clear position for Node.js on this topic is important. (please don't comment in that thread now unless you absolutely have to - the point being to try to get some internal consensus on the matter)

@bmeck
Copy link
Member

bmeck commented Feb 12, 2022

I'm not sure we should add WHATWG issues to our meeting if they are not interested in Node's feedback from what I read in the comments.

@guybedford
Copy link

guybedford commented Feb 12, 2022 via email

@GeoffreyBooth
Copy link
Member

I’ve added whatwg/html#5572 to the agenda. I put it first in case @guybedford doesn’t want to stay for the ambient loaders discussion. (@guybedford I assume you can make the meeting?)

@GeoffreyBooth
Copy link
Member

it may help to have a unified Nodejs position as to whether we are able to make the resolver synchronous or not.

I think the question we need to answer is this: what do we lose, if anything, if we make the resolve hook synchronous? Currently Node’s internal version, defaultResolve, is async but doesn’t do anything asynchronous, and so it could be made async without consequence. Likewise, all of the examples of loaders in this repo and in the Node docs show sample resolve hooks that are defined as async functions but don’t do anything async, and could be changed into sync functions. I looked at the source for ts-node and the same seems to apply there too.

So it would seem that we could make resolve sync and aside from the churn of the breaking change, there would be no loss of functionality and no abandoned use cases. Does that hold true for the future, as well? Are there any potential use cases for an async resolve, that we want to preserve the capability to support in the future? If so, what are they?

@arcanis
Copy link
Contributor

arcanis commented Feb 15, 2022

Are there any potential use cases for an async resolve, that we want to preserve the capability to support in the future? If so, what are they?

Softwares that want to run a bunch of resolutions in parallel (TypeScript, Webpack, etc) would want resolvers to be async, that's at least part of why they reimplement the resolution themselves at the moment.

Although such softwares also tend to have many other reasons to implement their own resolution pipelines as they add features like types etc that would require to extend require.resolve in ways that aren't possible today, it would be nice if someday this wasn't necessary...

@JakobJingleheimer
Copy link
Contributor

Currently Node’s internal version, defaultResolve, is async but doesn’t do anything asynchronous

That's not the case as of HTTP(S) Imports: defaultResolve() calls defaultGetFormatWithoutErrors(), and in the case of a network protocol, calls getHttpProtocolModuleFormat() (which is async, although it may settle immediately).

@GeoffreyBooth
Copy link
Member

That’s not the case as of HTTP(S) Imports: defaultResolve() calls defaultGetFormatWithoutErrors(), and in the case of a network protocol, calls getHttpProtocolModuleFormat() (which is async, although it may settle immediately).

Okay, but it doesn’t have to, right? I thought load was responsible for definitively determining format, and resolve only provided it if it happened to already know what it was; but determining the format wasn’t something that resolve needed to do.

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Feb 15, 2022

Actually, yes. Why is it doing that at all… 🤔

Ah, I know why: it was part of nodejs/node#40980. I think as it currently is, when the import is a network protocol, defaultResolve()'s format will be an unsettled promise.

Maybe when getHttpProtocolModuleFormat() is called with ignoreErrors set to true, getHttpProtocolModuleFormat() should just abort (returning null)?

@cspotcode
Copy link
Contributor

Is this related to the possibility of making resolve sync from the main thread while keeping the implementation possibly async in the loader thread? I'm not intimately familiar with the topic, but I think there's a desire to expose all loader hooks as sync in the main thread to support loading of CJS files.

@JakobJingleheimer
Copy link
Contributor

Is this related to the possibility of making resolve sync from the main thread while keeping the implementation possibly async in the loader thread?

Yes

I'm not intimately familiar with the topic, but I think there's a desire to expose all loader hooks as sync in the main thread to support loading of CJS files.

I don't understand—it already can load CJS file.

@cspotcode
Copy link
Contributor

cspotcode commented Feb 15, 2022

I thought someone on the loaders team was talking about allowing loaders to implement both resolve() and load() behind a require() call. So the main thread can make a blocking require() call, and it can be dispatched to the loader's thread, where resolve() and load() are allowed to asynchronously determine the module's source text, running a compiler in the loader thread, for example.

@JakobJingleheimer
Copy link
Contributor

I don't recall that specifically, but if using the atomic wizardry mentioned recently, that sounds possible (not sure if it's a good idea or something that might be considered specifically supporting if it doesn't JustWork after loaders are moved off-thread).

@bmeck
Copy link
Member

bmeck commented Feb 15, 2022

@JakobJingleheimer this was one of the use cases for moving things off thread and Modules WG have talked about it for years, some edge cases do exist with doing so though but they would be a follow on not a blocker.

@GeoffreyBooth
Copy link
Member

I thought someone on the loaders team was talking about allowing loaders to implement both resolve() and load() behind a require() call.

This sounds like the effort a few years back of trying to make require of an ESM file possible. A PR was opened to achieve that via low-level C++ trickery, not atomics wizardry, so maybe this atomics approach might be a new way to potentially solve that problem? (The C++ version didn’t pass code review.)

@arcanis
Copy link
Contributor

arcanis commented Feb 15, 2022

For the record, this is the kind of implementation for the "syncify" part of the implementation (here, for babel-register):

https://github.com/babel/babel/blob/e77e3de402693a216088cb75cdf6a245cfd7d0f8/packages/babel-register/src/worker-client.js#L69-L85

@cspotcode
Copy link
Contributor

to make require of an ESM file possible.

That seems like a different thing: blocking the main thread while simultaneously spinning the main thread's event loop to evaluate an ESM module.

Rather, this would be blocking the main thread waiting on the loader thread to return a path and source text to be executed. Execution would still happen synchronously on the main thread, and an error would be thrown if load() returned format: esm.

@brev
Copy link
Contributor

brev commented Feb 18, 2022

Hello Node Loaders Team and community,

FYI, I've recently used the current Loaders API to build a new method of testing web code under Node.js, for the bleeding-edge SvelteKit web framework.

The create-esm-loader and node-esm-loader wrappers helpers help bring stability and sanity.

Details:
sveltejs/kit#19 (comment)

Loader sources:
https://github.com/brev/esm-loaders

So far, I have been able to achieve everything I need. Some of it is not ideal, but I think your planned work covers most of it already.

In case it's helpful. Thanks.

@GeoffreyBooth
Copy link
Member

Do you mind submitting a PR to https://github.com/nodejs/loaders/blob/main/doc/resources.md to add these links?

@brev
Copy link
Contributor

brev commented Feb 21, 2022

@GeoffreyBooth sure thing, done here: #68. thanks.

@davidje13
Copy link

I see a lot of discussion in this thread is about whether resolve could be made synchronous without losing functionality.

Specifically to answer:

So it would seem that we could make resolve sync and aside from the churn of the breaking change, there would be no loss of functionality and no abandoned use cases. Does that hold true for the future, as well? Are there any potential use cases for an async resolve, that we want to preserve the capability to support in the future? If so, what are they?

I'd like to share a few places where I have found resolve being asynchronous to be useful (I've noted other possible approaches for all of these, along with potential downsides);

File system operations (checking existence, permissions)

It's possible to use the sync versions of these operations (accessSync / statSync / etc.) but of course these block the thread (which is currently shared with the main NodeJS process, so blocking it is not desirable when a dynamic import is triggered while the program is already running). Also using promise-based versions is better for reusability (e.g. my own approach shares code between a NodeJS loader and a webserver which performs pre-processing when serving resources for browser-based testing)

Loading js-based config

A common pattern is for tools to use a *.js file to get their configuration (see e.g. Rollup, WebPack). This config is loaded by simply loading the file using import or require. For import, this necessarily requires promises.

It would be possible to use top-level await to load config like this in advance, but that assumes that we always know where to look for them. In reality, we often want to look for config files once we know the path of the file we are loading from.

Other dynamic importing

For example a loader which decides to load typescript only when it finds a .ts file or Babel only when it finds a .jsx file. This could be achieved by loading it in the load method then assuming its presence in subsequent resolve calls, but that could also get quite complicated. (this is not an actual use-case for me but is something I considered)

Network requests

Again not a use-case for me, but I could imagine a loader which fetches resources over the network. In this case, an import like ./foo would need to perform a network request to find out what the file extension should be (ignoring this and just resolving to plain foo then handling everything in load would be problematic as an import for ./foo in one location and ./foo.js in another would not be recognised as being the same module)

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

Successfully merging a pull request may close this issue.

9 participants