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.

Sign up for GitHub

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

Jump to bottom

module: add createRequire method #27405

Closed
wants to merge 2 commits into from
Closed

module: add createRequire method #27405

wants to merge 2 commits into from

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Apr 25, 2019 •
edited
Loading

This is an abstraction on top of creatRequireFromPath that can accept
both paths, URL Strings, and URL Objects.

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

@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3363

@MylesBorins MylesBorins force-pushed the createRequire branch 2 times, most recently from a83b861 to 8c6a784 Compare April 25, 2019 06:20
benjamingr
benjamingr approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

@mscdex
Copy link
Contributor

mscdex commented Apr 25, 2019

Is there a reason this can't be a part of createRequireFromPath() instead of creating a separate function?

@targos targos added module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 25, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22737/

@MylesBorins
Copy link
Contributor Author

Updated code based on feedback from @devsnek. Have also added documentation to modules.md.

@mscdex I don't think this makes sense to add to createRequireFromPath as that API explicitly states it is meant for paths. I think it might make sense to deprecate createRequireFromPath and make it an internal API, as createRequire is far more useful IMHO.

@MylesBorins MylesBorins mentioned this pull request Apr 26, 2019
Closed
jkrems
jkrems approved these changes Apr 26, 2019
View reviewed changes
Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22739/

@MylesBorins
Copy link
Contributor Author

CI is actually green the fail above is a lie. If there are no objections to this PR I plan to land it as is on Monday

Fishrock123
Fishrock123 approved these changes Apr 26, 2019
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me... (but I am not convinced that it is an acceptable replacement of import.meta.require...)

bnoordhuis
bnoordhuis reviewed Apr 28, 2019
View reviewed changes
doc/api/modules.md Show resolved Hide resolved
test/parallel/test-module-create-require.js Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Contributor Author

Changed a few things here:

  • createRequire now only supports absolute paths (either path or URL)
    • this is because the underlying logic in Module for relative paths is undefined / confusing e.g. caching cwd(). This limitation, which we could choose to remove later, guards against unexpected behavior (which was present in our documentation)
  • createRequireFromPath is now docs deprecated
    • please LMK if I need to do anything more, haven't deprecated before

PTAL

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22843/

@sam-github sam-github mentioned this pull request May 1, 2019
Closed
@MylesBorins
Copy link
Contributor Author

One more stab at it

Differences:

  • now fails with non-url objects early
  • swallows all errors from fileURLToPath and has a single unified error
  • explicitly only support
    • file URL objects
    • file URL string
    • absolute path string

I believe this has scoped the use of this API enough that it guards against most footguns PTAL

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
BridgeAR
BridgeAR reviewed May 1, 2019
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really deprecate the other API, it'll need an entry in doc/api/deprecations.md.

doc/api/modules.md Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Show resolved Hide resolved
@mscdex mscdex mentioned this pull request May 2, 2019
Closed
@benjamingr
Copy link
Member

changes still lgtm

@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label May 3, 2019
Trott
Trott reviewed May 3, 2019
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22917/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22933/

@MylesBorins
Copy link
Contributor Author

latest CI was green!

landed in 411063c

@MylesBorins MylesBorins closed this May 3, 2019
MylesBorins added a commit that referenced this pull request May 3, 2019
@MylesBorins
module: add createRequire method
411063c 
This is an abstraction on top of creatRequireFromPath that can accept
both paths, URL Strings, and URL Objects.

PR-URL: #27405
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 4, 2019
@MylesBorins @targos
module: add createRequire method
5bcd770 
This is an abstraction on top of creatRequireFromPath that can accept
both paths, URL Strings, and URL Objects.

PR-URL: #27405
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request May 6, 2019
Merged
targos added a commit that referenced this pull request May 6, 2019
@targos
2019-05-07, Version 12.2.0 (Current)
492762e 
Notable changes:

* tls:
  * Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
    option to `tls.createServer()`. When enabled, TSL packet trace
    information is written to `stderr`. This can be used to debug TLS
    connection problems. #27497
* cli:
  * Added a `--trace-tls` command-line flag that enables tracing of TLS
    connections without the need to modify existing application code.
    #27497
  * Added a `--cpu-prof-interval` command-line flag. It can be used to
    specify the sampling interval for the CPU profiles generated by
    `--cpu-prof`. #27535
* module:
  * Added the `createRequire()` method. It allows to create a require
    function a file URL object, a file URL string or an absolute path
    string. The existing `createRequireFromPath()` method is now
    deprecated #27405.
  * Throw on `require('./path.mjs')`. This is technically a breaking
    change that should have landed with Node.js 12.0.0. It is necessary
    to have this to keep the possibility for a future minor version to
    load ES Modules with the require function.
    #27417
* meta:
  * Added Christian Clauss (https://github.com/cclauss) to
    collaborators. #27554

PR-URL: #27578
targos added a commit that referenced this pull request May 7, 2019
@targos
2019-05-07, Version 12.2.0 (Current)
e8dda76 
Notable changes:

* deps:
  * Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
    parser refuse any request URL that contained the "|" (vertical bar)
    character. #27595
* tls:
  * Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
    option to `tls.createServer()`. When enabled, TSL packet trace
    information is written to `stderr`. This can be used to debug TLS
    connection problems. #27497
    #27376
* cli:
  * Added a `--trace-tls` command-line flag that enables tracing of TLS
    connections without the need to modify existing application code.
    #27497
  * Added a `--cpu-prof-interval` command-line flag. It can be used to
    specify the sampling interval for the CPU profiles generated by
    `--cpu-prof`. #27535
* module:
  * Added the `createRequire()` method. It allows to create a require
    function from a file URL object, a file URL string or an absolute
    path string. The existing `createRequireFromPath()` method is now
    deprecated #27405.
  * Throw on `require('./path.mjs')`. This is technically a breaking
    change that should have landed with Node.js 12.0.0. It is necessary
    to have this to keep the possibility for a future minor version to
    load ES Modules with the require function.
    #27417
* repl:
  * The REPL now supports multi-line statements using `BigInt` literals
    as well as public and private class fields and methods.
    #27400
  * The REPL now supports tab autocompletion of file paths with `fs`
    methods. #26648
* meta:
  * Added Christian Clauss (https://github.com/cclauss) to
    collaborators. #27554

PR-URL: #27578
targos added a commit that referenced this pull request May 7, 2019
@targos
2019-05-07, Version 12.2.0 (Current)
e006a85 
Notable changes:

* deps:
  * Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
    parser refuse any request URL that contained the "|" (vertical bar)
    character. #27595
* tls:
  * Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
    option to `tls.createServer()`. When enabled, TSL packet trace
    information is written to `stderr`. This can be used to debug TLS
    connection problems. #27497
    #27376
* cli:
  * Added a `--trace-tls` command-line flag that enables tracing of TLS
    connections without the need to modify existing application code.
    #27497
  * Added a `--cpu-prof-interval` command-line flag. It can be used to
    specify the sampling interval for the CPU profiles generated by
    `--cpu-prof`. #27535
* module:
  * Added the `createRequire()` method. It allows to create a require
    function from a file URL object, a file URL string or an absolute
    path string. The existing `createRequireFromPath()` method is now
    deprecated #27405.
  * Throw on `require('./path.mjs')`. This is technically a breaking
    change that should have landed with Node.js 12.0.0. It is necessary
    to have this to keep the possibility for a future minor version to
    load ES Modules with the require function.
    #27417
* repl:
  * The REPL now supports multi-line statements using `BigInt` literals
    as well as public and private class fields and methods.
    #27400
  * The REPL now supports tab autocompletion of file paths with `fs`
    methods. #26648
* meta:
  * Added Christian Clauss (https://github.com/cclauss) to
    collaborators. #27554

PR-URL: #27578
This was referenced May 7, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
@MylesBorins @aduh95
module: add createRequire method
8beaac3 
This is an abstraction on top of creatRequireFromPath that can accept
both paths, URL Strings, and URL Objects.

PR-URL: nodejs/node#27405
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek left review comments

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax left review comments

@SMotaal SMotaal SMotaal left review comments

@guybedford guybedford guybedford left review comments

@bnoordhuis bnoordhuis bnoordhuis left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@Trott Trott Trott approved these changes

@jkrems jkrems jkrems approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
module Issues and PRs related to the module subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.