docs: Update contributing doc #2026
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2026 +/- ##
=======================================
Coverage 90.97% 90.98%
=======================================
Files 146 145 -1
Lines 7492 7454 -38
Branches 1502 1488 -14
=======================================
- Hits 6816 6782 -34
+ Misses 676 672 -4 |
|
|
||
| - [NPM](https://npmjs.com) | ||
| - [TypeScript](https://www.typescriptlang.org/) | ||
| - [lerna](https://github.com/lerna/lerna) to manage dependencies, compilations, and links between packages. Most lerna commands should be run by calling the provided npm scripts. |
There was a problem hiding this comment.
I think this is out of date now. AFAIK lerna is only now used for (a) lerna run ... because that supports parallel execution of a command in many workspace package dirs and (b) lerna publish ... for the release process (though I'm not sure on this one).
Perhaps other devs use other lerna ... commands for their work?
There was a problem hiding this comment.
I'll be honest, I'm not positive exactly how lerna and npm workspaces work together in this repo. I noticed lerna is still in commands in the package.json which is what led me to believe it's still in use for various things... but I do know that running lerna commands directly (instead of the npm command that calls lerna) causes problems and doesn't work as expected so I'm very much open to someone telling me what I have wrong here! 😅
There was a problem hiding this comment.
Yah, I had hoped to drop lerna entirely when starting using npm workspaces, but npm run ... does not support the parallel running that lerna run ... does. Also lerna ... was being used -- at least I think so -- in some of the release process that I don't know well at all.
There was a problem hiding this comment.
Yes, as far as I know lerna is used to:
- run scripts in parallel
- lint/lint:fix
- updating the
version.tsfiles for the current package and all it's local dependenciesI wonder though: do we even need this in contrib? I know we use it in core, but I just checked and I didn't see any usages of the version file in any of our packages. Maybe it's redundant. I'll open a follow-up issue to investigate thisEdit: I must have selected the wrong scope when searching for usages. It's actually quite widely used to set the instrumentation version.
- updating versions on release (used by release-please, it might be okay with it just being there in
package.json) - releasing the packages (lerna publish, we might be able to replace this with an npm counterpart)
CONTRIBUTING.md
Outdated
|
|
||
| The `opentelemetry-js-contrib` project is written in TypeScript. | ||
|
|
||
| - `npm install` to install dependencies. | ||
| As a general rule, installing and then compiling from the root directory should always be done first before anything else. | ||
| After installing and compiling from the root directory, do it again from the specific package directory you are working in. |
There was a problem hiding this comment.
do it again from the specific package directory you are working in.
Do you find that is necessary after a build at the top-level? It is after making any local changes, but not otherwise in my experience so far.
There was a problem hiding this comment.
At this point I realized I've been doing it out of habit, so it's certainly possible that it's only necessary to do it after local changes. So maybe something like always install and compile from the root, and then install and compile in the local package after any changes are made
There was a problem hiding this comment.
OK I've updated to clarify this a bit better
|
|
||
| ### Tools used | ||
|
|
||
| - [NPM](https://npmjs.com) |
There was a problem hiding this comment.
Everything below is automatically installed, right? Might be good to mention what versions of npm/node are supported/recommended, or possibly have something like a nvmrc file or similar?
There was a problem hiding this comment.
Good callout - I cheated a little to avoid this getting out-of-date, and suggested links to refer to for dev dependencies and runtimes.
|
|
||
| - [NPM](https://npmjs.com) | ||
| - [TypeScript](https://www.typescriptlang.org/) | ||
| - [lerna](https://github.com/lerna/lerna) to manage dependencies, compilations, and links between packages. Most lerna commands should be run by calling the provided npm scripts. |
There was a problem hiding this comment.
Yes, as far as I know lerna is used to:
- run scripts in parallel
- lint/lint:fix
- updating the
version.tsfiles for the current package and all it's local dependenciesI wonder though: do we even need this in contrib? I know we use it in core, but I just checked and I didn't see any usages of the version file in any of our packages. Maybe it's redundant. I'll open a follow-up issue to investigate thisEdit: I must have selected the wrong scope when searching for usages. It's actually quite widely used to set the instrumentation version.
- updating versions on release (used by release-please, it might be okay with it just being there in
package.json) - releasing the packages (lerna publish, we might be able to replace this with an npm counterpart)
Which problem is this PR solving?
Short description of the changes