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

doc: add Customization section and subsections to related sub systems #50419

Closed

Conversation

JakobJingleheimer
Copy link
Member

This is a split from #49704 with just doc updates (the code change in 49704 will subsequently require 1 more sentence added to the docs added here).

@JakobJingleheimer JakobJingleheimer added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Oct 26, 2023
@JakobJingleheimer JakobJingleheimer self-assigned this Oct 26, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@JakobJingleheimer
Copy link
Member Author

@GeoffreyBooth I checked npm, and there are currently no packages registered with the keyword nodejs-plugin. I think nodejs-customization as an entry point should be sufficiently unique (AFAIK, there is no way to check).

Copy link
Member

@GeoffreyBooth GeoffreyBooth 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’re standardizing on nodejs-plugin for the package.json keywords field, then why is the standardized export nodejs-customization? Shouldn’t these be the same?

I don’t think we should involve --env-file in this unless we land a semver-major change to go out in a future version of Node that automatically loads .env. Otherwise it’s just an extra step with no purpose, and you could cut it out and replace the “use --env-file=.env“ part with “use --import nodejs-plugin/nodejs-customization“. Also because I think it’s likely that we build on --env-file to add support for a legit config file, such as one in JSON, and that would be a better place to persist this than in an .env file; especially if a future version of Node autoloads that config file.


> Stability: 1.0 - Early development

Node.js has multiple APIs for customizing behaviour and can easily be invoked
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Node.js has multiple APIs for customizing behaviour and can easily be invoked
Node.js has multiple APIs for customizing behavior and can easily be invoked

Copy link
Member Author

@JakobJingleheimer JakobJingleheimer Oct 29, 2023

Choose a reason for hiding this comment

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

Gah. I think VS Code uses my system language for spell check. (This project has confused the heck outta spell check on my iPhone)

Copy link
Member

Choose a reason for hiding this comment

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

I use this extension: https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker

It allows you to have custom settings by workspace, so you can create a Node workspace and configure cSpell within that workspace to use US English.

Comment on lines 24 to 27
support it provides; for a plugin extending support for typescript would contain
`typescript` in its keywords; avoid red-herrings such as `typescript` as a
keyword when the library does not extend support for typescript but is merely
itself written in typescript. Failing to avoid these red-herrings will cause
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
support it provides; for a plugin extending support for typescript would contain
`typescript` in its keywords; avoid red-herrings such as `typescript` as a
keyword when the library does not extend support for typescript but is merely
itself written in typescript. Failing to avoid these red-herrings will cause
support it provides; a plugin adding Node.js support for TypeScript should contain
`typescript` in its keywords, to avoid matches such as `typescript` as a
keyword when the library does not add support for TypeScript but is merely
itself written in TypeScript. Using nonstandard keywords will cause

itself written in typescript. Failing to avoid these red-herrings will cause
your package to be listed for things it does not do, aggravating users.

These steps configure Node.js with a customization plugin:
Copy link
Member

Choose a reason for hiding this comment

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

This section mixes instructions for package authors writing a plugin with instructions for using such a plugin. I feel like those should be separate sections, as end users aren’t going to care about how a plugin should be authored.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a heading ("Setting up a plugin") between 28 and 30

Copy link
Member

@GeoffreyBooth GeoffreyBooth Oct 30, 2023

Choose a reason for hiding this comment

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

We should probably put the “Using a plugin” section first, as it applies to more people. (Not sure if “setting up” means authoring or consuming.)

search`][], as well as a website.
1. Once installed, in order to get Node.js to automatically use it, create a
[`.env`][`--env-file`] file containing an [`--import`][] for your chosen
plugin, like `NODE_OPTIONS="--import=example-nodejs-plugin"`.
Copy link
Member

Choose a reason for hiding this comment

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

If we’re using the standardized export that you mentioned above, this would be

Suggested change
plugin, like `NODE_OPTIONS="--import=example-nodejs-plugin"`.
plugin, like `NODE_OPTIONS="--import=example-nodejs-plugin/nodejs-customization"`.

Copy link
Member Author

@JakobJingleheimer JakobJingleheimer Oct 29, 2023

Choose a reason for hiding this comment

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

OH!! Actually, this is an interesting point, and I think absolves us from a not-great entry-point name: We "control" this --import, so if the --import itself specifies the entry-point, I think that means it's safe to use whatever we want (so this would be okay):

{
  "name": "example-nodejs-plugin",
  "exports": {
    "register": "whatever.mjs"
  },
  "keywords": ["nodejs-plugin", ""]
}
NODE_OPTIONS="--import=example-nodejs-plugin/register"

The problem before would exist if node automatically calls the entry-point whenever any package is --imported; but if we include the entry-point in the --import's specifier, then we're safe (and this only happens when the user manually puts it there, or in future, when the wizard automates it).

Copy link
Member

Choose a reason for hiding this comment

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

Right, but that’s also why it’s not that important that it be standardized. I think the only benefit of standardization is so that the wizard we eventually create knows what export to use.

Copy link
Member Author

Choose a reason for hiding this comment

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

[…] the wizard we eventually create knows what export to use.

Yes, exactly.


* A [`resolve` hook][resolve hook] that sets `format` for applicable modules to
the format it handles, such as `'typescript'`.
* A [`load` hook][load hook] that transpiles the external format (as signalled
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* A [`load` hook][load hook] that transpiles the external format (as signalled
* A [`load` hook][load hook] that transpiles the external format (as signaled

@JakobJingleheimer
Copy link
Member Author

If we’re standardizing on nodejs-plugin for the package.json keywords field, then why is the standardized export nodejs-customization? Shouldn’t these be the same?

nodejs-plugin seems more likely to be an entry-point (but that can't be confirmed). We could make the keyword and the entry-point key the same, but I think in that case, we need to go with nodejs-customization.

RE --env-file. We've had this conversation several times. The reason is because the documentation is explaining how to manually do a process that we plan to automate, in the way it will be automated, so the user doesn't have to do anything else once it's automated. If/when node starts automatically loading the env file, the user can just drop --env-file from their command.

AFAIK, there is no design or implementation in progress (only some stalled discussions) to support a different kind of config file.

@GeoffreyBooth
Copy link
Member

If/when node starts automatically loading the env file, the user can just drop --env-file from their command.

AFAIK, there is no design or implementation in progress (only some stalled discussions) to support a different kind of config file.

There’s no design or implementation in progress for automatically loading .env either. I wouldn’t assume that that’s coming in some future version of Node, if ever.

@JakobJingleheimer
Copy link
Member Author

There’s no design or implementation in progress for automatically loading .env either. I wouldn’t assume that that’s coming in some future version of Node, if ever.

True. But it's what we've got, which is better than what we don't. I think it's still a better experience. I added both options with explanations for both.

@GeoffreyBooth
Copy link
Member

But it’s what we’ve got, which is better than what we don’t.

I think these instructions need to be essentially the algorithm for the wizard; the wizard just automates these steps. And what if the user already has a .env file? Or is already using --env-file? Or their command to run Node already includes --import? Or the command to run Node includes NODE_OPTIONS=something? That’s a lot of complexity for the wizard to handle.

@JakobJingleheimer
Copy link
Member Author

I think these instructions need to be essentially the algorithm for the wizard

Yes, that is the intention. Yet you keep suggesting/requesting things that are contrary to that.

I imagine we would need a util that reads the current .env file (if it exists), builds a map of all the args (grouping common args), appends the new --import, and then flattens the map. It would surely be easier with a package.json-like config file, but .env is what we have 😕

Comment on lines +21 to +27
Setting the keyword `nodejs-plugin` is important for users to find the
package (which may be automated). It should also contain the official name(s)
for the support it provides; a plugin extending Node.js support for TypeScript
should contain `typescript` in its keywords; a customization package that does
not extend Node.js support for TypeScript should not contain `typescript` (such
as when the library is itself merely written in TypeScript). Incorrect keywords
will cause a package to be listed for things it does not do, aggravating users.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm not fan of documenting non-technical patterns in the Node documentation.

Adding the npm keyword is useless from a technical perspective, whether users want to use them or not is more a subjective guideline than a "you must do this for the loader to be compatible with Node.js".

Copy link
Member

Choose a reason for hiding this comment

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

The plan is that it would be a requirement for the wizard we plan to build, so that the wizard can find packages to suggest. But to your point, perhaps it’s better to exclude this until the wizard actually exists, and any steps that are required specifically for wizard compatibility (as opposed to general Node compatibility) could be added later as part of the PR that adds the wizard.

Copy link
Member Author

@JakobJingleheimer JakobJingleheimer Nov 5, 2023

Choose a reason for hiding this comment

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

I'm not fan of documenting non-technical patterns in the Node documentation.

@arcanis are you talking about this part?

Incorrect keywords will cause a package to be listed for things it does not do, aggravating users.

I'm trying to avoid users complaining (to node) of erroneous results from authors throwing in red-herrings.


What is described here is critical for the automated process (in whatever form). To limit changes for authors, these instructions need to be available as soon as possible.

Adding the npm keyword is useless from a technical perspective, whether users want to use them or not is more a subjective guideline than a "you must do this for the loader to be compatible with Node.js".

I'm confused—this seems to directly contradict what you told me:

I think uses the npm tags as part of the search, so if all loaders have similar tags that will probably help

So unless what Geoffrey said above (that tags are just keywords lifted from package.json) is incorrect, this is necessary. Or have I missed something completely?

The tag needs to be "claimed" immediately, regardless of how it gets there.

1. Install the plugin via your preferred package manager. For finding the
plugin, package managers provide often a CLI utility, such as [`npm
search`][], as well as a website.
2. Once installed, in order to get Node.js to use it, you have 2 options:
Copy link
Contributor

Choose a reason for hiding this comment

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

A third option for Yarn users is to add the NODE_OPTIONS variable to .env.yarn, which gets automatically injected into all yarn node calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, this may be more convenient, but if it works for only a minority of uses, I think it would be better for node to have 1 implementation that works for all.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Oct 30, 2023

Yet you keep suggesting/requesting things that are contrary to that.

No, I’m suggesting that it be simplified as much as possible, so that a wizard can achieve it. Adding --env-file to the mix makes it more complicated than it needs to be. It also means that users would need to use --env-file, but they might not want to; they might already be using dotenv, which is more feature-complete than --env-file, and it’s error-prone to use both. They also might want to use --env-file for certain modes, like running the app, that differ from the modes they want to register hooks for, such as running the app in dev mode and running tests.

The definitive source of how an app is run is the package.json scripts field. The wizard can ask questions like “which package.json script(s) would you like to add TypeScript support to?” (once it’s already been established that the user wants to add TypeScript support). Then for such commands, NODE_OPTIONS=--import=whatever can be prepended to the command. We could simply error if NODE_OPTIONS is already present in the script, asking the user to do it themselves, unless we want to take on the challenge of merging together whatever their NODE_OPTIONS might already specify with what we want to add.

This also covers adding support for tests but possibly not for production, say, like if the user wants this support added to a dev script but not a start script. (And again, they might use --env-file for all modes, to load environment variables for start, but they might not want hooks registered in production.) The wizard would need to do this editing of the package.json scripts work anyway to add --env-file, so it’s simplifying things to just add --import there instead of --env-file.

Alternatively we can expand on --env-file and build a new feature like --config-file that loads a proper JSON config file, and land that first and build this on top of that; and make the usage of a Node config file a prerequisite for the wizard. That feels like where you’re going with wanting to require .env files, and it seems reasonable as then the defined configuration is editable; the wizard can go back to it on a second run to adjust things. I just don’t think --env-file is that, and it’s better to stick with package.json by itself than to try to make .env files into something they’re not.

@JakobJingleheimer
Copy link
Member Author

users would need to use --env-file

Huh? That's not true at all O.o --env-file is merely one way to get the config from .env loaded into node.

but they might not want to; they might already be using dotenv

Users so inclined can merely do in lieu of specifying --env-file—these instructions don't preclude using dotenv at all. We can include such a note in this doc (dotenv users, specify -r dotenv/config instead of --env-file), but that seems a rabbit-hole.

The definitive source of how an app is run is the package.json scripts field. The wizard can ask questions like “which package.json script(s) would you like to add TypeScript support to?” (once it’s already been established that the user wants to add TypeScript support). Then for such commands, NODE_OPTIONS=--import=whatever can be prepended to the command.

That sounds mostly six of one, half a dozen of the other, with the pjson scripts route slightly more complicated. But if that gets this feature landed, I'm very content to accept that and end these months of wheel-spinning.

build a new feature like --config-file that loads a proper JSON config file

IMO that would be the ideal. I remember there were several enthusiastic advocates of this during the discussions for .env. If we can land that, great! I think that would be a big win on its own. Buuut, I think it would be a very foolish gamble to delay "claiming" the npm tag we need until after that config file feature lands (nodejs-plugin is currently available and is about as concise as it could get; if delay means having to find some awful alternative, I'll be extremely annoyed). If we want to go that route, before we start on that journey, we need to publish some form of this doc claiming/dictating the npm tags we'll need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants