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

Update dependency lerna to v7 #1542

Closed
1 task
pichlermarc opened this issue Jun 19, 2023 · 8 comments
Closed
1 task

Update dependency lerna to v7 #1542

pichlermarc opened this issue Jun 19, 2023 · 8 comments
Assignees

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Jun 19, 2023

The "bootstrap" command was removed by default in v7, and is no longer maintained., which means we'd have to change the way we link packages, possibly by using npm workspaces.

Further information:

See also:

Same issue in the otel-js core repo open-telemetry/opentelemetry-js#3894

@pichlermarc
Copy link
Member Author

In the long term we'll need to use another solution, but in the short term, can also go the route of adding @lerna/legacy-package-management, which will give us bootstrap back 🙂

See open-telemetry/opentelemetry-js#3926

@chigia001
Copy link
Contributor

chigia001 commented Jul 6, 2023

@pichlermarc should we switch to use npm workspaces in once PR?
I'm play arround with it a little bit in the current repo and foun the biggest issue is npm install in workspace don't guarantee the install order for each workspace => some workspace might depend on another's prepare script to excute first.
npm/cli#4139

We can solve this by remove prepare script in each workspace use lerna to trigger each workspace's compile script in correct order.

EDIT: also found some issue with hoisting, right now some workspace like instrumentation-user-interaction have a tsconfig file that reference a file from zone.js in node_modules. By default zone.js will be hoist to root dir and when compile instrumentation-user-interaction tsc will unable to find this file (they expect zone.js appear in instrumentation-user-interactio/node_modules). Lerna suggest that we use yarn if we require complicate hoisting rule.

@pichlermarc
Copy link
Member Author

@pichlermarc should we switch to use npm workspaces in once PR?

Yes, I think doing it in one go would be preferable, or at least in a way that allows us to still release after merging the PR. I think creating that PR will be quite painful due to the amount of possible conflicts. Another option would also be switching to rush as the folks over at https://github.com/open-telemetry/opentelemetry-sandbox-web-js/ use that. Really, the biggest goal is to future-proof our setup - with bootstrap on it's way out we need to find another solution so that we're not stuck with old tooling in the long run.

I'm play arround with it a little bit in the current repo and foun the biggest issue is npm install in workspace don't guarantee the install order for each workspace => some workspace might depend on another's prepare script to excute first. npm/cli#4139

We can solve this by remove prepare script in each workspace use lerna to trigger each workspace's compile script in correct order.

I think this makes sense, I wonder why we even have that prepare script here in contrib - I think we never had that setup in the core repo https://github.com/open-telemetry/opentelemetry-js and we require running compile first to do that AFAIK 🤔

EDIT: also found some issue with hoisting, right now some workspace like instrumentation-user-interaction have a tsconfig file that reference a file from zone.js in node_modules. By default zone.js will be hoist to root dir and when compile instrumentation-user-interaction tsc will unable to find this file (they expect zone.js appear in instrumentation-user-interactio/node_modules). Lerna suggest that we use yarn if we require complicate hoisting rule.

Hmm, I see. I'm not opposed to switching to yarn, though I'd also like to hear the other @open-telemetry/javascript-maintainers opinion on that before moving forward with it.

Thank you for taking care of this, it may also be worth to sync with @clesleycode, they're working on the same thing over at the core repo open-telemetry/opentelemetry-js#3927. Would you like for me to assign this issue to you? 🙂

@chigia001
Copy link
Contributor

chigia001 commented Jul 6, 2023

Would you like for me to assign this issue to you? 🙂

Yes, I already have a PR #1561.
Right now it working with some twist but not sure hoisting is feature we need in long term.

There still some issue with the PR need to fix.

@chigia001
Copy link
Contributor

I just learn more about the use of test-all-versions in the codebase and right now tav only support npm right now. So if we want to switch to other package manager that is something we also need to consider.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Copy link
Contributor

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Nov 20, 2023
Copy link
Contributor

This issue was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment