loaders icon indicating copy to clipboard operation
loaders copied to clipboard

Node.js Loaders Team Meeting 2023-07-18

Open mhdawson opened this issue 1 year ago • 9 comments

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:

  • https://www.timeanddate.com/worldclock/fixedtime.html?msg=Node.js+Foundation+Loaders+Team+Meeting+2023-07-18&iso=20230718T1530&p1=1440

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

mhdawson avatar Jul 14 '23 12:07 mhdawson

I think we should have this meeting, to coordinate the various PRs that we have in flight. Cross-posting from https://github.com/nodejs/node/pull/46662#issuecomment-1636837976 (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 https://github.com/nodejs/node/pull/48559#discussion_r1260335580); 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:

  • https://github.com/nodejs/node/pull/48559
  • The PR that comes out of https://github.com/nodejs/loaders/issues/147 (remove globalPreload, move its functionality into register / initialize); there are already branches for this but not yet opened as a PR.
  • https://github.com/nodejs/node/pull/47999

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 https://github.com/nodejs/node/pull/46662 and https://github.com/nodejs/node/pull/48439 and other PRs that are ready but not on our roadmap.

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

GeoffreyBooth avatar Jul 15 '23 17:07 GeoffreyBooth

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?

JakobJingleheimer avatar Jul 16 '23 15:07 JakobJingleheimer

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 https://github.com/nodejs/node/pull/48559#discussion_r1260335580.

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, https://github.com/nodejs/node/pull/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?

GeoffreyBooth avatar Jul 16 '23 17:07 GeoffreyBooth

I think 2 (https://github.com/nodejs/node/pull/48559 and https://github.com/nodejs/loaders/issues/147) of the 3 items listed above will likely land within the next couple weeks, but how close is https://github.com/nodejs/node/pull/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.

JakobJingleheimer avatar Jul 16 '23 20:07 JakobJingleheimer

  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?

arcanis avatar Jul 16 '23 20:07 arcanis

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 avatar Jul 16 '23 22:07 GeoffreyBooth

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 avatar Jul 16 '23 22:07 GeoffreyBooth

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 avatar Jul 17 '23 15:07 GeoffreyBooth

It appears that https://github.com/nodejs/node/pull/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 https://github.com/nodejs/node/pull/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, https://github.com/nodejs/node/pull/47999 and https://github.com/nodejs/node/pull/46662, don’t involve public-facing API changes and could perhaps land after the unflagging has been released.

GeoffreyBooth avatar Jul 17 '23 20:07 GeoffreyBooth