nodejs / TSC

The Node.js Technical Steering Committee
577 stars 127 forks source link

Additions to `import.meta` #1430

Open GeoffreyBooth opened 1 year ago

GeoffreyBooth commented 1 year ago

Opening this issue per today’s TSC meeting. This is a spinoff from https://github.com/nodejs/node/pull/49246#pullrequestreview-1585722251:

An import.meta.node namespace was floated in https://github.com/wintercg/proposal-common-minimum-api/issues/50#issuecomment-1664274693, but Bun objected. I think if we want to create a proprietary namespace then we need another WinterCG proposal for the namespace itself.

See also https://github.com/wintercg/proposal-common-minimum-api/pull/54.

richardlau commented 1 year ago
  • Should additions to this shared namespace require WinterCG and/or TSC approval?

I was under the impression that import.meta was intended to not be a shared namespace -- that implementors could add whatever made sense for their runtime. The issue then becomes if Javascript runtimes start adding useful things to it that are similar how do end users write code using them if they are not common across the runtimes? (e.g. if Bun and Node.js added different import.meta.* APIs to get the filename, how would someone write code that worked both in Bun and Node.js?)

benjamingr commented 1 year ago

Are additions to import.meta semver-major or semver-minor?

I think semver-minor is fine since unlike new globals I don't expect code to break when we add stuff to import.meta.

Should additions to this shared namespace require WinterCG and/or TSC approval?

I don't think we should require WinterCG approval for anything. I think we should engage with WinterCG in good faith and try to come up with standards and interoperability that work for all of us. I don't think it should block anything certainly not as a policy.

benjamingr commented 1 year ago

e.g. if Bun and Node.js added different import.meta.* APIs to get the filename, how would someone write code that worked both in Bun and Node.js?

In this case either Bun align (likely), Node.js would align (less but still likely) or someone would write an interop package for people who care about whether or not their code is portable across runtimes.

I do believe standardization is a good idea and asking for feedback is good.

GeoffreyBooth commented 1 year ago

I was under the impression that import.meta was intended to not be a shared namespace

Maybe that’s what TC39 intended, but like a few other things, that’s not what’s happened in practice. We went through years of negotiations with WHATWG to reach consensus on import.meta.resolve, and then through a yearlong PR to move loaders off-thread to match the import.meta.resolve spec defined by the HTML spec (among other reasons). I understand that there are some members of the community who feel that Node should just ignore WHATWG and other standards and do whatever we like, but I think it’s been a resolved policy for many years now that we try to adhere to standards with other runtimes whenever possible, especially whenever we implement APIs that exist in other runtimes.

The unfortunate thing is that import.meta is the only place to put APIs that want to know anything about the current module. For example, we recently added import { register } from 'node:module' and we would love to support register('./file.js') but instead users need to do register('./file.js', import.meta.url) or register(import.meta.resolve('./file.js')) because there’s no way that register can know the URL of the module that calls it; that’s just not possible, except for methods attached to import.meta. So now import.meta becomes this very prime real estate where API designers might want to attach anything that could benefit from knowing about the current or calling module; it’s not just a basket of generic static properties about the module.

In this case either Bun align (likely), Node.js would align (less but still likely) or someone would write an interop package for people who care about whether or not their code is portable across runtimes.

In general I think we want to encourage interoperability of code by default; there are plenty of authors who write packages for Node, or for browsers, that could be compatible across runtimes but because it’s not the path of least resistance, they neglect to do so. The broader user base is better served the more we can make it easy for packages to be written in a cross-compatible way by default.

What would this “interop package” look like? import.meta can only be accessed within the current module; it’s like __filename in that it’s specific to the current module and not shared with other modules. It therefore can’t be polyfilled, other than using module customization hooks to do things like rewrite the source of modules before they’re evaluated (which is a very Node-specific solution). The only reasonable way for users to write compatible code is via things like const absolutePath = import.meta.filename ?? import.meta.path (to cover Node and Bun); there’s no way to write a library to export a helper for this. These annoyances are why I think it’s especially important to adhere to standards around import.meta.

ljharb commented 1 year ago

fwiw, a helper would likely look something like wrap(import.meta), and then you'd access from or destructure that object the same as you would import.meta.

GeoffreyBooth commented 1 year ago

My takes:

  • Are additions to import.meta semver-major or semver-minor?

I think semver-minor.

  • Should additions to this shared namespace require WinterCG and/or TSC approval?

I think at least TSC approval, and the TSC should make an effort to get WinterCG approval too. We should also try to add whatever WHATWG adds to import.meta if we can (currently they’ve defined just import.meta.url and import.meta.resolve, both of which we support).

I also think that we should be sparing in granting approvals. There was a suggestion to add import.meta.register here, for import { register } from 'node:module', but to me that feels ridiculous: we shouldn’t be putting things here that are rarely used, just to avoid passing a second argument to a method. I think there’s probably also a performance cost in each thing that we add, as import.meta is an object created anew for every loaded module.

ljharb commented 1 year ago

I doubt there's actually a performance cost - {} vs { key: value } should be identically instantaneous, and adding a getter for something that needs computation should be negligible.

mhdawson commented 1 year ago

I think at least TSC approval, and the TSC should make an effort to get WinterCG approval too.

TSC approval aligns with my thinking at well with the expecation that the TSC should work to use the same values across runtimes when possible.

jsumners commented 1 year ago

I was under the impression that import.meta was intended to not be a shared namespace -- that implementors could add whatever made sense for their runtime.

Yep.

I think semver-minor

+1

I don't think we should require WinterCG approval for anything.

+1,000,000

I think we should engage with WinterCG in good faith and try to come up with standards and interoperability that work for all of us. I don't think it should block anything certainly not as a policy.

+1

I understand that there are some members of the community who feel that Node should just ignore WHATWG and other standards and do whatever we like

A couple of things from a non-maintainer perspective:

So now import.meta becomes this very prime real estate where API designers might want to attach anything that could benefit from knowing about the current or calling module;

That's literally the spec defined point of it, right?

In general I think we want to encourage interoperability of code by default; there are plenty of authors who write packages for Node, or for browsers, that could be compatible across runtimes but because it’s not the path of least resistance, they neglect to do so. The broader user base is better served the more we can make it easy for packages to be written in a cross-compatible way by default.

I really don't see the point. Non-browser runtimes are by definition not web browsers. It'd be an unmitigated disaster if web browsers all implemented different runtime environments because they are designed to view client agnostic web pages. That's the thing folks like WHATWG are governing. But non-browser runtimes are chosen by developers for the features (API) of those runtimes. I don't think it is unreasonable for those developers to be aware of the ramifications of their choices.

I think at least TSC approval,

Agreed.

and the TSC should make an effort to get WinterCG approval too.

Disagree.

benjamingr commented 1 year ago

From TSC discussion, it seems like we're gaining consensus around:

mcollina commented 1 year ago

@nodejs/tsc @jasnell are there any objections with this plan?


Are additions to import.meta semver-major or semver-minor?

semver-minor

Should additions to this shared namespace require WinterCG and/or TSC approval?

TSC approval, plus WinterCG notified

aduh95 commented 1 year ago

Can we clarify what TSC approval means in this context? Does that mean TSC should vote every time we want to add something to import.meta? Does that apply also to things we want to modify an existing property?

aduh95 commented 1 year ago

IMO requiring TSC approval (whatever that means) is really not necessary, IMO the usual code review process is enough. If there are objections/lack of consensus, the TSC can get involved, but I don't see the point if the addition is already at consensus. AFAICT the current review process works well enough, let's not complicate it further, wdyt? A better policy would be "TSC and WinterCG should be notified". TSC members (or any collaborator) can object to the PR / add the PR to the TSC agenda if needed.

GeoffreyBooth commented 1 year ago

IMO requiring TSC approval (whatever that means) is really not necessary, IMO the usual code review process is enough.

No, additions here need to be coordinated with other runtimes. That's something the TSC needs to be involved with, if not lead. We can't just have any two collaborators landing whatever they want on here.

benjamingr commented 1 year ago

I assumed "the TSC is pinged and we operate by lazy consensus and not necessarily a vote".

aduh95 commented 1 year ago

I assumed "the TSC is pinged and we operate by lazy consensus and not necessarily a vote".

If that's the case, +1, but that seems to be better described by "the TSC is notified" rather than "the TSC needs to approve".

That's something the TSC needs to be involved with, if not lead. We can't just have any two collaborators landing whatever they want on here.

The TSC does not own the code, the collaborators do. I don't know what your last sentence is implying, but I don't think I agree with it, the TSC members are not some kind of "better collaborators". Strong -1 on my side to add a vote requirement, it would be an overreach from the TSC, and it would add useless friction when the current process is actually working well enough.

benjamingr commented 1 year ago

The TSC does not own the code, the collaborators do

+100 to that

GeoffreyBooth commented 1 year ago

We can't just have any two collaborators landing whatever they want on here.

The TSC does not own the code, the collaborators do. I don’t know what your last sentence is implying, but I don’t think I agree with it, the TSC members are not some kind of “better collaborators”.

What I mean is that we shouldn’t have just two approvals be enough here. import.meta is a shared namespace and we need to be more careful with it, including outreach to groups like WinterCG. Making the TSC sign off in some way, like lazy consensus, both ensures that the outreach happens and also that whoever does the outreach can speak for the project as a whole.

We have a policy that any new global is considered a semver-major change, and per https://github.com/nodejs/node/blob/6919d7241657517bfff6d2041748fa9f8e85b76e/doc/contributing/collaborator-guide.md#breaking-changes:

At least two TSC voting members must approve backward-incompatible changes to the main branch.

I don’t think we need to make import.meta additions semver-major, but they should have a similar policy in requiring some kind of TSC sign-off.

aduh95 commented 1 year ago

import.meta is a shared namespace and we need to be more careful with it, including outreach to groups like WinterCG

Isn't that already what's happening with the current process? There have been several proposal of addition for import.meta, and none have landed except import.meta.resolve. What would have changed with the proposal?

GeoffreyBooth commented 1 year ago

Isn’t that already what’s happening with the current process? There have been several proposal of addition for import.meta, and none have landed except import.meta.resolve. What would have changed with the proposal?

I think the point of this issue is to formalize what so far has been an improvised process, so that we do the same for future import.meta proposals. They’ll only get more common, I think, as ESM adoption increases.

Especially with so many sources across the web defining import.meta as host-defined, many contributors might assume that we treat this particular API as much more open for proprietary contributions than we are, at least right now.

mhdawson commented 1 year ago

I'm in favor of @mcollina's suggestion in https://github.com/nodejs/TSC/issues/1430#issuecomment-1708352106 where TSC approval means the regular lazy consensus process.

jasnell commented 1 year ago

Changes to import.meta should likely be semver-minor. Changes to import.meta should require TSC approval.

I would suggest semver-major as it's a global namespace. That said, see below.

WinterCG should be consulted with and collaborated with in good faith but is not blocking.

The WinterCG has discussed setting up a kind of registry for import.meta terms. I have an outstanding todo to get that started this week. Essentially, WinterCG likely won't standardize the import.meta terms itself but will provide a registry (in the same sense, for instance, that IANA provides a registry for things like http headers, etc) that implementations can use to coordinate use of specific import.meta properties with the key idea being to encourage runtimes not to step on each other when introducing new properties.

With such a registry in place, if runtimes do agree to adhere to the registry and use it as a way of coordinating import.meta properties, then semver-minor is sufficient for adding new properties as the risk of incompatibilities is greatly reduced.

Automation as a concern was raised since we currently have automation for semver-major. In order to not miss import.meta changes we should should fix our automation so these changes don't get missed.

aduh95 commented 1 year ago

I'm going to say it explicitly: I'm -1 on requesting TSC approval for changes on import.meta. I think this is not a necessary change, and we should resist the TSC granting itself additional power unless it's actually necessary. I think the TSC should come up with guidelines on how to review a PR proposing changes to import.meta, and should trust the collaborators to follow those guidelines (and those guidelines are not followed for whatever reason, the change can be reverted immediately).

If WinterCG goes forward with their idea described in https://github.com/nodejs/TSC/issues/1430#issuecomment-1717937856, my suggestion for the guidelines would be:


Any change to import.meta cannot land unless:

It must also follow the usual process for landing PRs – specifically, that means a feature can be accepted on the WinterCG registry, and still be rejected in Node.js if a collaborator objects to it.


Ideally, the TSC, or at least someone from the project, should be involved in reviewing changes to the WinterCG registry though. IMO any change to that registry would deserve discussion from the TSC, maybe we can have an automation that automatically adds to the TSC agenda any new PR to the registry. We could add @nodejs/tsc as an "owner" of the file where import.meta properties are set, so the TSC members get notified when a change is suggested, but I'm not sure it's necessary, the above guidelines should be enough. We should probably NOT make a recommendation on what semverness import.meta changes should be, and instead let the usual process apply (semver-minor when it's an addition, semver-major when it can break someone, semver-patch if it's a fix).

wdyt?

jasnell commented 1 year ago

I'm still waiting on more feedback from WinterCG before we can consider this official, but I have the start of the registry here: https://github.com/wintercg/import-meta-registry

benjamingr commented 1 year ago

Changes to import.meta should likely be semver-minor. Changes to import.meta should require TSC approval.

I would suggest semver-major as it's a global namespace. That said, see below.

Why? What's the breaking potential - the issue with globals is they can change how JS code executes with regards to scoping - that's not a concern here as far as I'm aware?

GeoffreyBooth commented 1 year ago

I’m going to say it explicitly: I’m -1 on requesting TSC approval for changes on import.meta. I think this is not a necessary change, and we should resist the TSC granting itself additional power unless it's actually necessary. I think the TSC should come up with guidelines on how to review a PR proposing changes to import.meta, and should trust the collaborators to follow those guidelines (and those guidelines are not followed for whatever reason, the change can be reverted immediately).

I disagree with this. Ultimately every PR needs TSC approval, since any block could lead to a TSC vote. Requiring two TSC approvals on an import.meta PR is a way to ensure that the TSC has considered it, and it slows down the process which is necessary in this case because we need to ensure that the specific workflow around import.meta has been followed (opening the proposal on the WinterCG repo, etc.).

It’s not a question of trusting or not trusting the collaborators. It’s simply unreasonable to expect all of them to know about a particular process that’s specific to import.meta. It’s even unreasonable to expect everyone in @nodejs/tsc to necessarily remember that this process needs to be followed, or to react in time to block a PR until it’s done. Requiring two TSC approvals slows a PR down enough to hopefully ensure that someone who remembers what needs to be done has ensured that it happens before the PR lands.

Additions to import.meta should be rare. It’s okay if they’re difficult to land. The more populated that namespace becomes, the more interoperability and portability problems we create.

aduh95 commented 1 year ago

@GeoffreyBooth do you have any opinions on the guidelines I have drafted?

GeoffreyBooth commented 1 year ago

@GeoffreyBooth do you have any opinions on the guidelines I have drafted?

Yes. The guideline should include that two TSC approvals are required, like we have for globals.

I also think that requiring only that a PR be open for 7 days in the WinterCG repo is way too lax. It should be landed there. The registry is just a place for runtimes to lay claim to names; there’s no reason for other runtimes to ever block a proposal other than “hey, we already use that name.” There’s also no hurry on landing import.meta PRs in Node. We can wait for the WinterCG process to play out.

benjamingr commented 1 year ago

As mentioned above in https://github.com/nodejs/TSC/issues/1430#issuecomment-1690326415 I object to requiring WinterCG approval for additions, we are placing boundaries on ourselves that none of the other runtime are committing to and we have the biggest obligation to our users out of all of them as the biggest runtime.

Node.js should do what's best for Node.js and the web and should collaborate in good faith with WinterCG but not be subservient to it IMO. Like Node.js can add stuff to import.meta without WHATWG or TC39 approval...

I also agree with TSC approval (through lazy consensus, since we have globality concerns) but haven't heard a strong argument for semver-major. I don't see it as different from adding an argument to a method which isn't major. I am also OK with "regular collaborator process + ping".

GeoffreyBooth commented 1 year ago

I object to requiring WinterCG approval for additions

We can add some kind of condition like “if rejected at WinterCG, or still pending at WinterCG after two months, a lazy consensus or vote of the TSC can decide to proceed without WinterCG approval.” I would be fine with that. But WinterCG only meets once per month so it might take up to two months for a decision to be made by them. (But again, I don’t think it should be easy or quick to land additions to import.meta.)

I agree that regular semver rules should apply, that additions are presumed minor, changes to existing properties are probably major, etc.

benjamingr commented 1 year ago

@GeoffreyBooth mostly, people in WinterCG today (like James) are great and I trust them but as a standards body it's new and I wouldn't want our process to block on it or wait for it.

What about the following idea:

The changed requirement is that "import.meta additions cannot move from experimental without either WinterCG registration or TSC approval" which addresses standardisation concerns and "collaborators own the code' concerns (both I think are valid). It also means we explicitly seek WInterCG feedback on import.meta changes which I think is a good thing.

GeoffreyBooth commented 1 year ago

That’s mostly good, I would change a few things:

aduh95 commented 1 year ago

The idea of having a separate team dedicated to this SGTM – probably not a WG, and possibly with a broader scope than just import.meta, maybe @nodejs/interoperability or something.