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

Backporting util/types, path/posix and path/win32 to v15 proposal #35788

Closed
ExE-Boss opened this issue Oct 24, 2020 · 11 comments
Closed

Backporting util/types, path/posix and path/win32 to v15 proposal #35788

ExE-Boss opened this issue Oct 24, 2020 · 11 comments

Comments

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Oct 24, 2020

Is your feature request related to a problem? Please describe.

I’d like to get #34055 and #34962 backported to v15.x.


Note that no backport PRs exist yet, and the above PRs were only merged into v16.0.

Describe the solution you'd like

I propose the following options:

  1. Backport util: add util/types alias module #34055 and path: add path/posix and path/win32 alias modules #34962 as‑is to v15, which, as an odd numbered release, is considered an unstable non‑LTS release, and thus some minor breakage might be somewhat acceptable for it.

  2. Backport util: add util/types alias module #34055 and path: add path/posix and path/win32 alias modules #34962 to v15, but restrict them to the node: scheme until v16, with the following sub‑options:

    1. Add support for require("node:<built‑in‑id>") and thus require("node:util/types"), require("node:path/posix") and require("node:path/win32"). (see also: Add support for node:‑prefixed imports of built‑in modules to require(…) #36098)

    2. Don’t add support for require("node:<built‑in‑id>"), thus only supporting import("node:util/types"), import("node:path/posix") and import("node:path/win32"), but not require("node:util/types"), require("node:path/posix") and require("node:path/win32")

@ExE-Boss ExE-Boss changed the title Backporting util/types, path/unix and path/win32 to v15 proposal Backporting util/types, path/posix and path/win32 to v15 proposal Oct 24, 2020
@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Oct 24, 2020
@aduh95
Copy link
Contributor

aduh95 commented Oct 24, 2020

v15, which, as an odd numbered release, is considered an unstable non‑LTS release

I don't think that's true, v15 should not be considered unstable – the odd numbered release rule was in place in the early days of Node.js 0.x IIRC, nowdays Node.js follows semver for the most part. Stability should not be the focus anyway, breaking changes have been landed on even numbered releases too (E.G.: #27417 landed in v12.2.0).

Correct me if I'm wrong, the PR at stake here are not introducing breaking changes, but they do add a new core module which is the reason they got labeled semver-major (based on discussion around #34055 (comment)).

Here's what I could find in the docs regarding the add of a new modules in core:

### Introducing new modules
Treat commits that introduce new core modules with extra care.
Check if the module's name conflicts with an existing ecosystem module. If it
does, choose a different name unless the module owner has agreed in writing to
transfer it.
If the new module name is free, register a placeholder in the module registry as
soon as possible. Link to the pull request that introduces the new core module
in the placeholder's `README`.
For pull requests introducing new core modules:
* Allow at least one week for review.
* Land only after sign-off from at least two TSC members.
* Land with a [Stability Index][] of Experimental. The module must remain
Experimental until a semver-major release.

Nothing here stipulates the PR MUST be considered as semver-major, I'd say it SHOULD land as semver-minor in v15 because:

  • the new core modules are not "top-level", they uses already-reserved names (path and util).
  • both PRs checks all the points in the above policy (well, maybe it would require a backport PR adding the experimental status first).

@ljharb
Copy link
Member

ljharb commented Oct 24, 2020

util/types luckily doesn't conflict with https://unpkg.com/browse/util@0.12.3/, the browserify shim for util. path/posix and path/win32luckily do not conflict with https://unpkg.com/browse/path@0.12.7/ either. In general though, there's no such thing as a "reserved name" when it comes to appending file paths on the end.

There's still the chance someone in an enterprise app has named a private package, or manually added a dir, that conflicts with these module names - obviously it's exceedingly unlikely.

If it's decided to backport them, I can ensure that resolve resolves them properly prior to the release PR landing, so CIGTM can pass.

@Trott
Copy link
Member

Trott commented Oct 24, 2020

@nodejs/tsc @nodejs/releasers @nodejs/modules-active-members

@Trott
Copy link
Member

Trott commented Oct 25, 2020

Just in case I'm not at the TSC meeting: I'm in favor of backporting these changes to the 15.x line.

@targos
Copy link
Member

targos commented Oct 25, 2020

Is there anyone against that?

@mhdawson
Copy link
Member

+1 from me.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Nov 11, 2020

@mhdawson
This shouldn’t be closed yet, as #34055 and #34962 haven’t yet been backported to v15.x.

@targos
Copy link
Member

targos commented Nov 11, 2020

I removed the semver-major label from those PRs. They'll be picked up automatically when the next release is prepared.

@ljharb
Copy link
Member

ljharb commented Nov 11, 2020

@targos is this a general policy change about new core modules being semver-major? Or were those two PRs included in the one-off decision about the diagnostics module?

@mhdawson
Copy link
Member

@ExE-Boss k, thanks I will remove from the TSC agenda instead

@mhdawson mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Nov 11, 2020
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Nov 30, 2020

This has now shipped in v15.3.0 (#34055).

Follow‑up work to add support for node:‑prefixed imports to require(…) is being tracked in #36098.

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

7 participants