node icon indicating copy to clipboard operation
node copied to clipboard

module: add --package flag for implicit config testing

Open bmeck opened this issue 3 years ago β€’ 14 comments

refs: https://github.com/nodejs/node/discussions/37857

bmeck avatar Apr 01 '21 16:04 bmeck

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

bmeck avatar Apr 01 '21 16:04 bmeck

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.

bmeck avatar Apr 01 '21 18:04 bmeck

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=?

ljharb avatar Apr 01 '21 18:04 ljharb

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.

bmeck avatar Apr 01 '21 18:04 bmeck

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

jasnell avatar Apr 01 '21 19:04 jasnell

cc @nodejs/modules

GeoffreyBooth avatar Apr 02 '21 23:04 GeoffreyBooth

@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 avatar Apr 05 '21 13:04 bmeck

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.

bmeck avatar Apr 05 '21 14:04 bmeck

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?

ljharb avatar Apr 05 '21 14:04 ljharb

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

aduh95 avatar Apr 05 '21 14:04 aduh95

@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 avatar Apr 05 '21 14:04 bmeck

@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?

bmeck avatar Apr 05 '21 14:04 bmeck

@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 :)

aduh95 avatar Apr 05 '21 15:04 aduh95

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.

bmeck avatar May 27 '21 19:05 bmeck