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 2023-07-18 #149

Open
mhdawson opened this issue Jul 14, 2023 · 9 comments
Open

Node.js Loaders Team Meeting 2023-07-18 #149

mhdawson opened this issue Jul 14, 2023 · 9 comments
Assignees

Comments

@mhdawson
Copy link
Member

mhdawson commented Jul 14, 2023

Time

UTC Tue 18-Jul-2023 15:30 (03:30 PM): 8:30 am PT / 15:30 UTC / 5:30 pm CEST

Or in your local time:

Links

Agenda

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

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

@GeoffreyBooth
Copy link
Member

I think we should have this meeting, to coordinate the various PRs that we have in flight. Cross-posting from nodejs/node#46662 (comment) (cc @aduh95), here’s my current thinking on a possible plan for the next few steps of developing the Loaders API. (Open to feedback and revisions!)

I feel like our top priority is to complete Milestone 2, the items in our roadmap that we feel like we need to get done in order to mark the Loaders API as stable. We’re very close to this currently; there are PRs or branches for all three items on the list, and we just need to get them landed and released. Some of these PRs would unflag the Loaders API, unless we introduce a new flag to gate it (see nodejs/node#48559 (comment)); but I think if we keep everything unreleased until we land everything for Milestone 2, then release them altogether, we can just unflag the implementation rather than create a new flag. Currently register is already landed but unreleased (marked as dont-backport) and the remaining PRs could be the same. There are only three of them:

Once all of these land and then get released, the Loaders API can become stable as soon as we’ve squashed all the bugs that people report. We can also land nodejs/node#46662 and nodejs/node#48439 and other PRs that are ready but not on our roadmap.

Open to feedback, and/or we can discuss in the meeting.

@JakobJingleheimer
Copy link
Contributor

Mm, let's do :)

I'm leaning towards not holding everything back from release because it would be useful to users even without "stable". Is the only reason we would need to do that because of the --experimental-loader flag?

@GeoffreyBooth
Copy link
Member

I’m leaning towards not holding everything back from release because it would be useful to users even without “stable”. Is the only reason we would need to do that because of the --experimental-loader flag?

See nodejs/node#48559 (comment).

I think these are our options:

  1. Hold back the last few PRs and release them all at once along with removing the need for a flag. The API would still print a warning and stay experimental in the docs. Some might argue that this isn’t really a change from the status quo because --loader already exists (without needing experimental).
  2. For whichever PR removes the need for --experimental-loader / --loader, we add some new boolean flag that is required to enable the loaders API (or to enable register specifically). Something like --experimental-module-register say. This would probably be the very next PR to land, esm: unflag Module.register and allow nested loader import() node#48559. If we go with this option, then that PR and the preceding register one can get released immediately and each subsequent PR on our list can also go out as soon as they land.

I’m leaning toward the first option because I think the list of PRs that would be held back is short; I’m hoping they can all be landed in the next few weeks, and since Node is only released once every two weeks or so, the difference between the two options is minor (like maybe register and the new flag goes out in the first release and the rest of the list goes out in the second release that’s two weeks later). The benefit of the first option is less churn for end users; the changes come out together, and there’s no new flag to learn about and adopt.

On the other hand if the remaining PRs on the list take months to ship, then that argues more for option 2 since this is a prolonged process and we should get stuff into users’ hands sooner. But since we already have code written for all of these items, I feel like this is less likely?

@JakobJingleheimer
Copy link
Contributor

I think 2 (nodejs/node#48559 and #147) of the 3 items listed above will likely land within the next couple weeks, but how close is nodejs/node#47999? @aduh95 would you still like to tag-team that? I have time this week and a bit this coming weekend (then I'm away 26 Jul – 05 Aug).

less churn for end users

register is additive, so no churn there, so I think the only churn is globalPreloadregister, right?

there’s no new flag to learn about and adopt

I think whatever we do, we should not introduce a new flag for register.

@arcanis
Copy link
Contributor

arcanis commented Jul 16, 2023

  1. For whichever PR removes the need for --experimental-loader / --loader, we add some new boolean flag that is required to enable the loaders API (or to enable register specifically). Something like --experimental-module-register say.

Assuming you don't mean to remove altogether support for --experimental-loader and --loader (which I think would be a mistake, considering it's widely known, already part of many documentations, and would make it difficult to use loaders in a way that would work on both <future Node.js version> and <first LTS release tools have to support>), can't we just make them imply --experimental-module-register and remove them once all LTS releases support register?

@GeoffreyBooth
Copy link
Member

Assuming you don’t mean to remove altogether support for --experimental-loader and --loader

No, I don’t mean actually removing the flags, I just mean removing the need for them because users can use register instead.

At some point we might want to deprecate --loader because it’s redundant now that we have register, but that can come later. Replacing it is as simple as --import 'data:text/javascript,import { register } from "module"; register("my-loader")' though most popular packages will probably have a register export, so you can use them like --import ts-node/register or similar.

@GeoffreyBooth
Copy link
Member

register is additive, so no churn there, so I think the only churn is globalPreloadregister, right?

I think whatever we do, we should not introduce a new flag for register.

The churn would be if we introduce a new flag. And yes, removing globalPreload would also be churn, but since that hook is so rarely used it shouldn’t be too big of an impact.

@GeoffreyBooth
Copy link
Member

Per https://openjs-foundation.slack.com/archives/C053UCCP940/p1689559741493429 we’re moving this 2.5 hours earlier, to 8:30 am PT / 15:30 UTC / 5:30 pm CEST.

@GeoffreyBooth
Copy link
Member

It appears that nodejs/node#48559 should be able to land soon, basically as soon as we can get passing CI. I marked it as “don’t release” because it would unflag the Loaders API. In the “next steps” proposals above I was assuming that we wouldn’t want to unflag the Loaders API until all the Milestone 2 PRs were landed, but maybe we don’t need to wait for all of them? Since some don’t involve public-facing API changes.

We could follow up nodejs/node#48559 with the globalPreload PR; once both of these are landed, we could release them along with the previous register PR that’s currently landed but unreleased. The remaining PRs in flight, nodejs/node#47999 and nodejs/node#46662, don’t involve public-facing API changes and could perhaps land after the unflagging has been released.

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

No branches or pull requests

4 participants