module: add --package flag for implicit config testing #38028

bmeck · 2021-04-01T16:35:20Z

DerekNonGeneric

Hmm, --package really? This is technically called a “package manifest” file while “package” more broadly refers to all of the package contents as well. Can we name this flag something else?

benjamingr · 2021-04-01T16:53:15Z

lib/internal/bootstrap/pre_execution.js

    } catch {}
  }
+  const implicitPackageJSONPath = getOptionValue('--package');


Yeah I agree with @DerekNonGeneric probably package-json-file or similar?

I'd prefer not package-json since the target could be named something like module-package.test we could go for package-file, does that seem fine?

I think --package-config would be preferable since that is what it is called elsewhere.

node/lib/internal/errors.js

Line 1183 in 3ef9562

E('ERR_INVALID_PACKAGE_CONFIG', (path, base, message) => {

How about --package-path and then also making it optional to include the package.json part?

@guybedford I'm not liking the excluding of the package.json since it is part of the path and we don't do searching generally these days. Additionally if we are fully explicit I wonder if we could do a inline-src like @aduh95 mentioned in #37848 (comment) for other stuff. If we do searching having it map directly to a URL would be weirder.

benjamingr · 2021-04-01T16:53:41Z

lib/internal/bootstrap/pre_execution.js

+      typeof cwdPackage.string === 'string' ||
+      typeof entryPackage.string === 'string'
+    ) {
+      throw ERR_PACKAGE_NOT_ALLOWED();


This seems like a reasonable tradeoff for now.

bmeck · 2021-04-01T16:53:49Z

It isn't called a manifest anywhere in Node.js core to my knowledge. We generally call it a config.

benjamingr

This is obviously missing a bunch of stuff (tests/docs) but I think this is a good enabler for the use case mentioned in #37857 while addressing most concerns so +1 on the solution.

guybedford

Seems useful!

guybedford · 2021-04-01T17:48:32Z

lib/internal/modules/package_json_reader.js

+ * @param {string} jsonPath 
+ * @param {string} srcPath 
+ */
+function clobber(jsonPath, srcPath) {


How about just making this set, and then providing the direct JSON to set into the cache? Since read is called before the usage of clobber anyway. This then also opens the door to possible inline package configuration options.

inline package configuration??

Yeah! (eg node app.js --pcfg.type=module as a natural extension)

ah, i was imagining just allowing a source text for the file since manually configuring multiple of them would be noise for the args parser

Yeah I wouldn't want to speculate, suggestion was as much about read + clobber = read + read + write -> read + write being the simplification.

guybedford · 2021-04-01T17:50:29Z

lib/internal/bootstrap/pre_execution.js

    } catch {}
  }
+  const implicitPackageJSONPath = getOptionValue('--package');


How about --package-path and then also making it optional to include the package.json part?

bmeck · 2021-04-01T18:36:54Z

looks like we have some naming ideas floating around, perhaps we should try and identify the components and what they bring:

package, the flag relates to a package (the entry package)
json, the flag expects to be provided JSON

It sure does, I doubt we will ever support anything else.

path, the value it takes is a path

This might not always be true, if we did allow URLs it would be possible to have inline sources / non-paths

config, the value represents the config for the package.

Overall I like --package-config the most, it is terse and doesn't bring up questions about having more types (URL w/ body vs just a file path) of values nor does it bring up questions about what would happen if we support non-JSON. Likely though, we should do an eager check of the JSON and error if this flag is provided. Right now malformed package.json files are ignored it seems after some checking, and we could match that but that could lead to misbehavior when attempting to ensure some kind of configuration with this flag I would think.

ljharb · 2021-04-01T18:52:40Z

So, to be clear, the intention is that this flag accepts "package.json contents", and only works when no other package.json contents would be available?

I don't think anyone calls package.json "config" in the ecosystem, it's just called "package.json". The file serves many purposes; it's a dependency manifest, it's config, it's metadata, etc.

Why not --package-json= or --package-json-contents=?

bmeck · 2021-04-01T18:57:29Z

So, to be clear, the intention is that this flag accepts "package.json contents", and only works when no other package.json contents would be available?

Yup, that is what the discussion thread referenced seemed to lean towards as an overall potential solution to the issue.

lib/internal/modules/package_json_reader.js

jasnell · 2021-04-01T19:02:30Z

Seems like a good idea. Obviously there's some bikeshedding, docs, and tests needed but I'm +1 on the idea.

GeoffreyBooth · 2021-04-02T23:30:01Z

cc @nodejs/modules

bmeck · 2021-04-05T13:57:06Z

@ljharb

--package-json

I think this would be the first flag that declares its format in its name, not directly opposed since it likely will never ever change but it seems inconsistent. I think it is somewhat fine but might lead people to thinking it only works with files named package.json

--package-json-contents

This one likely isn't a good idea to me. In particular if it states "contents" I would expect the value to be the contents and not a path to get the contents from.

bmeck · 2021-04-05T14:01:40Z

How do people feel about allowing this to be a URL (not talking HTTP support) so that data: works? This would be the first path parameter that takes a URL value. URLs do have an overlap with file paths so dealing with things like percent encoding get a bit tricky and wouldn't match --require or the main entry point resolver.

ljharb · 2021-04-05T14:26:36Z

Ah, see, i misunderstood then :-) i thought it was passing json on the command line. Isn’t the whole point of the flag to not have to make a separate file solely to add type:module?

As for making it a url, that seems very strange. If we want the contents of the file to be passed inline, why not allow that directly?

aduh95 · 2021-04-05T14:39:39Z

For reference, loaders are loaded by a URL rather than a path, which allows inlining:

node --experimental-loader 'data:text/javascript,export async function getFormat() { return { format: "module" } }' ./index.js

bmeck · 2021-04-05T14:50:25Z

@ljharb the discussion around the need for this feature is for testing purposes, it has static configuration of those package.json contents so I presumed the ability to pass files would be nicer than always accepting JSON, also command lines limit total characters per command usually in different OS, so a full dump of a package.json could be problematic

bmeck · 2021-04-05T14:57:11Z

@aduh95 I rather like that vs having 2 flags that take 2 different types which is why I think it could be useful to use here. Did you have an opinion on using a URL vs a file path?

aduh95 · 2021-04-05T15:05:33Z

@bmeck my feeling is using URLs is more future proof – as you said, HTTP support is not on the table, but maybe one day. I agree that we should try to make only one flag for this. That being I don't feel strongly either way, and I'd like to hear other's opinions :)

bmeck · 2021-05-27T19:02:23Z

I've been trying to get in consistent contact with TS about this but have failed except some indirect talk. Going to start moving forward w/o their feedback.

module: add --package flag for implicit config testing

19bd154

bmeck added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Apr 1, 2021

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 1, 2021

DerekNonGeneric reviewed Apr 1, 2021

View reviewed changes

benjamingr reviewed Apr 1, 2021

View reviewed changes

guybedford reviewed Apr 1, 2021

View reviewed changes

jasnell reviewed Apr 1, 2021

View reviewed changes

lib/internal/modules/package_json_reader.js Show resolved Hide resolved

jasnell reviewed Apr 1, 2021

View reviewed changes

lib/internal/modules/package_json_reader.js Show resolved Hide resolved

aduh95 mentioned this pull request Jul 24, 2021

module: disable CommonJS support if entry point is ESM #39508

Closed

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

molllyn1 mentioned this pull request Jun 8, 2023

[Snyk] Fix for 4 vulnerabilities molllyn1/node#79

Open

This was referenced Jun 8, 2023

[Snyk] Fix for 4 vulnerabilities kissofire/node#98

Open

[Snyk] Fix for 4 vulnerabilities aliceUnhinged613/node#93

Open

ron-a20 mentioned this pull request Jun 8, 2023

[Snyk] Fix for 4 vulnerabilities ron-a20/node#96

Open

baby636 mentioned this pull request Jun 9, 2023

[Snyk] Fix for 4 vulnerabilities baby636/node#93

Open

137717unity mentioned this pull request Jun 9, 2023

[Snyk] Fix for 4 vulnerabilities 137717unity/node#96

Open

wedataintelligence mentioned this pull request Jun 9, 2023

[Snyk] Fix for 4 vulnerabilities wedataintelligence/node#78

Open

kissofire mentioned this pull request Jun 25, 2023

[Snyk] Fix for 28 vulnerabilities kissofire/node#102

Open

kissofire mentioned this pull request Sep 28, 2023

[Snyk] Fix for 1 vulnerabilities kissofire/node#103

Open

molllyn1 mentioned this pull request Sep 28, 2023

[Snyk] Fix for 1 vulnerabilities molllyn1/node#83

Open

ron-a20 mentioned this pull request Sep 28, 2023

[Snyk] Fix for 1 vulnerabilities ron-a20/node#99

Open

137717unity mentioned this pull request Sep 28, 2023

[Snyk] Fix for 1 vulnerabilities 137717unity/node#100

Open

aliceUnhinged613 mentioned this pull request Sep 28, 2023

[Snyk] Fix for 1 vulnerabilities aliceUnhinged613/node#96

Open

baby636 mentioned this pull request Sep 28, 2023

[Snyk] Fix for 1 vulnerabilities baby636/node#105

Open

wedataintelligence mentioned this pull request Sep 29, 2023

[Snyk] Fix for 1 vulnerabilities wedataintelligence/node#82

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: add --package flag for implicit config testing #38028

module: add --package flag for implicit config testing #38028

bmeck commented Apr 1, 2021

DerekNonGeneric left a comment •

edited

Loading

benjamingr Apr 1, 2021

bmeck Apr 1, 2021

DerekNonGeneric Apr 1, 2021 •

edited

Loading

guybedford Apr 1, 2021

bmeck Apr 1, 2021

benjamingr Apr 1, 2021

bmeck commented Apr 1, 2021

benjamingr left a comment

guybedford left a comment

guybedford Apr 1, 2021

bmeck Apr 1, 2021

guybedford Apr 1, 2021

bmeck Apr 1, 2021

guybedford Apr 1, 2021

guybedford Apr 1, 2021

bmeck commented Apr 1, 2021

ljharb commented Apr 1, 2021

bmeck commented Apr 1, 2021

jasnell commented Apr 1, 2021

GeoffreyBooth commented Apr 2, 2021

bmeck commented Apr 5, 2021

bmeck commented Apr 5, 2021

ljharb commented Apr 5, 2021

aduh95 commented Apr 5, 2021

bmeck commented Apr 5, 2021

bmeck commented Apr 5, 2021

aduh95 commented Apr 5, 2021

bmeck commented May 27, 2021

module: add --package flag for implicit config testing #38028

Are you sure you want to change the base?

module: add --package flag for implicit config testing #38028

Conversation

bmeck commented Apr 1, 2021

DerekNonGeneric left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerekNonGeneric Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Apr 1, 2021

benjamingr left a comment

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Apr 1, 2021

ljharb commented Apr 1, 2021

bmeck commented Apr 1, 2021

jasnell commented Apr 1, 2021

GeoffreyBooth commented Apr 2, 2021

bmeck commented Apr 5, 2021

bmeck commented Apr 5, 2021

ljharb commented Apr 5, 2021

aduh95 commented Apr 5, 2021

bmeck commented Apr 5, 2021

bmeck commented Apr 5, 2021

aduh95 commented Apr 5, 2021

bmeck commented May 27, 2021

DerekNonGeneric left a comment •

edited

Loading

DerekNonGeneric Apr 1, 2021 •

edited

Loading