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

chore: switch to 'npm workspaces' #156

Merged
merged 3 commits into from
Oct 18, 2023
Merged

chore: switch to 'npm workspaces' #156

merged 3 commits into from
Oct 18, 2023

Conversation

trentm
Copy link
Member

@trentm trentm commented Oct 18, 2023

Let's try npm workspaces for handling multiple packages. The primary win is developing local changes to the helpers package and having the others automatically linked.

Note that npm workspaces requires npm@7 or later, which typically means node@16 or later. For earler Node.js versions there are utils/run-* scripts to support install/test/lint.

Let's try npm workspaces for handling multiple packages.
The primary win is developing local changes to the helpers
package and having the others automatically linked.

Note that npm workspaces requires npm@7 or later, which
typically means node@16 or later. For earler Node.js versions
there are utils/run-* scripts to support install/test/lint.
@trentm trentm self-assigned this Oct 18, 2023
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Oct 18, 2023
@trentm trentm marked this pull request as ready for review October 18, 2023 20:13
@david-luna
Copy link
Member

I've tested make all to see what's the result:

  • package-lock.json files are generated on each package. Maybe we want to ignore them
  • packages are not linked between them, instead the have a downloaded version from the registry

The second point may difficult to do changes in more than one package at the same time (I think they can manually nom link). We may want to highlight that in the contributing guide. This is a nitpick

@trentm
Copy link
Member Author

trentm commented Oct 18, 2023

I've tested make all to see what's the result:

* `package-lock.json` files are generated on each package. Maybe we want to ignore them

What version of node and npm were you using with this?
With node <16 and npm <7 this is the expected behaviour.

With node 18 I get:

% npm --workspaces install
...

% npm ls
@elastic/ecs-logging-nodejs@ /Users/trentm/el/ecs-logging-nodejs4
├─┬ @elastic/[email protected] -> ./packages/ecs-helpers
│ ├── @hapi/[email protected]
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected]
│ └── [email protected]
├─┬ @elastic/[email protected] -> ./packages/ecs-morgan-format
│ ├── @elastic/[email protected] deduped -> ./packages/ecs-helpers
│ ├── [email protected] deduped
│ ├── [email protected] deduped
│ ├── [email protected]
│ ├── [email protected] deduped
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected] deduped
│ └── [email protected] deduped
├─┬ @elastic/[email protected] -> ./packages/ecs-pino-format
│ ├── @elastic/[email protected] deduped -> ./packages/ecs-helpers
│ ├── @types/[email protected]
│ ├── [email protected] deduped
│ ├── [email protected] deduped
│ ├── [email protected] deduped
│ ├── [email protected] deduped
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected] deduped
│ ├── [email protected] deduped
│ └── [email protected] deduped
└─┬ @elastic/[email protected] -> ./packages/ecs-winston-format
  ├── @elastic/[email protected] deduped -> ./packages/ecs-helpers
  ├── [email protected] deduped
  ├── [email protected] deduped
  ├── [email protected]
  ├── [email protected] deduped
  ├── [email protected] deduped
  ├── [email protected] deduped
  ├── [email protected] deduped
  ├── [email protected] deduped
  └── [email protected]

@trentm trentm merged commit de643de into main Oct 18, 2023
7 checks passed
@trentm trentm deleted the trentm/npm-workspaces branch October 18, 2023 22:34
@david-luna
Copy link
Member

With node <16 and npm <7 this is the expected behaviour.

Then it's okay. It will be obvious if someone with node < 16 and npm < 7 creates a PR and accidentally includes those files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants