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

[v10.x] Backport createRequire to v10 #34950

Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 27, 2020

This PR adds the createRequire API to Node.js v10 – which currently only has the deprecated createRequireFromPath available.

Note that this does not backport the deprecation of createRequireFromPath, given the fact that v10 is on maintenance mode, I figured that would not make sense to introduce a deprecation at this point.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

MylesBorins and others added 3 commits August 27, 2020 22:44
This is an abstraction on top of creatRequireFromPath that can accept
both paths, URL Strings, and URL Objects.

PR-URL: nodejs#27405
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#27629
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Update the example to use import and import.meta.url instead
of require() and require.resolve().

PR-URL: nodejs#27762
Fixes: nodejs#27758
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added tools Issues and PRs related to the tools directory. v10.x labels Aug 27, 2020
@aduh95 aduh95 changed the title Backport createRequire to v10 [v10.x] Backport createRequire to v10 Aug 27, 2020
@GeoffreyBooth
Copy link
Member

Does this really work? The ES modules implementation in < 12 was significantly different than what shipped in 12.

@mcollina
Copy link
Member

I don't think we should ship new features on 10.x

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 27, 2020

Does this really work?

I does work, at least the tests pass.

I don't think we should ship new features on 10.x

No hard feeling if this gets rejected, I don't think that feature gets used a lot on v10 anyway (because ESM implementation is still behind a flag).

@jkrems
Copy link
Contributor

jkrems commented Aug 27, 2020

Does this really work? The ES modules implementation in < 12 was significantly different than what shipped in 12.

I think createRequire is valuable completely independent of ESM. It's an official API to require relative to arbitrary locations which previously required ugly workarounds using custom Module instances that weren't properly documented. It's useful for things like framework and/or CLI tools that want to require relative to some project root.

That being said, I kind of agree with @mcollina: It feels late to add new features to v10. It's been out of active LTS for a bit and has less than a year left before it goes out of support. This isn't a critical bug fix, so it likely shouldn't go into v10.

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 28, 2020

This isn't a critical bug fix, so it likely shouldn't go into v10.

If this are the rules, maybe we could add in our process to delete labels such as backport-requested-v10.x once a release line has reached maintenance mode? The backporting guide gives two rules on opening backport PR to LTS staging branches:

  • when the original PR is labeled backport-requested-vN.x
  • when the commits have matured for at least 2 weeks on the Current releases.

The original PR for this backport meets both criteria, I think our guides and label system shouldn’t encourage to create invalid PRs.

## What needs to be backported?
If a cherry-pick from master does not land cleanly on a staging branch, the
releaser will mark the pull request with a particular label for that release
line (e.g. `backport-requested-vN.x`), specifying to our tooling that this
pull request should not be included. The releaser will then add a comment
requesting that a backport pull request be made.
## What can be backported?
The "Current" release line is much more lenient than the LTS release lines in
what can be landed. Our LTS release lines (see the [Release Plan][])
require that commits mature in the Current release for at least 2 weeks before
they can be landed in an LTS staging branch. Only after "maturation" will those
commits be cherry-picked or backported.

@richardlau
Copy link
Member

If this are the rules, maybe we could add in our process to delete labels such as backport-requested-v10.x once a release line has reached maintenance mode?

@aduh95 That sounds reasonable (cc @nodejs/lts). Pull request along those lines welcome.

If you haven't seen it, https://github.com/nodejs/Release#release-phases describes the release phases and the general guidelines as to what goes in. We've tried to keep general policy in the Release WG repository and mechanics (i.e. the how-tos) as guides in this repository.

@aduh95 aduh95 closed this Sep 4, 2020
@aduh95 aduh95 deleted the backport-createrequire-to-v10 branch July 16, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants