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: update lerna to v7 and migrate to npm workspace #1561

Closed

Conversation

chigia001
Copy link
Contributor

@chigia001 chigia001 commented Jul 6, 2023

Which problem is this PR solving?

Short description of the changes

  • Add "workspaces" definition to root package.json
  • Remove the "prepare" script in each workspace's package.json
  • Clean up gts as it does not use anymore; I just happened to see this because its part of the lerna bootstrap arguments
  • Fix any hoist issue that occurred with the new setup:
    • @types/sinon<10.0.14 depend on @sinonjs/fake-timers which install a very old version of sinon => this cause some compile issue
    • zone.js => instrumentation-user-interaction depends on zone.js's type, but the way the type is defined in zone.js is generated, it can't be used with the import statement. We must use /// <reference syntax so that we don't need to fix the path to zone.js/dist/zone.js.d.ts
  • Update GH's action cache using setup-node
    • This will cache the ~/.npm instead of node_modules folder and won't cache any generated package-lock.json
      • this previous setup might create a cache lock file that we unable to replicate on local machine
      • as we also don't use yarn bootstrap anymore, the previous best practice for node lerna cache not apply for this case
  • Remove lerna:link in .tav.yml file

There might be some hoist issues in the future, and the current npmcli script doesn’t provide enough support for us for more complex hoisting like what lerna used to provide.

Next Step

As lerna@7 is just provide some additional feature on top of nx, ee might consider to include nx.json to increase caching between task and apply for nx-cloud. Nx-cloud should be free for OSS project like opentelemetry-js-contrib. Those change might also fix issue with #1473

@chigia001
Copy link
Contributor Author

the default node-14 env is bundle with npm@6's which seem not support npm workspace.
I'm looking into node-18's unit test issue.

@chigia001 chigia001 mentioned this pull request Jul 6, 2023
1 task
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #1561 (24fe745) into main (b5fc0c4) will decrease coverage by 4.29%.
The diff coverage is 92.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1561      +/-   ##
==========================================
- Coverage   96.06%   91.77%   -4.29%     
==========================================
  Files          14      137     +123     
  Lines         914     7064    +6150     
  Branches      199     1424    +1225     
==========================================
+ Hits          878     6483    +5605     
- Misses         36      581     +545     
Impacted Files Coverage Δ
...opentelemetry-instrumentation-fastify/src/utils.ts 87.50% <ø> (ø)
...de/instrumentation-cucumber/src/instrumentation.ts 92.30% <92.30%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 98.78% <100.00%> (ø)
plugins/node/instrumentation-cucumber/src/types.ts 100.00% <100.00%> (ø)

... and 119 files with indirect coverage changes

@chigia001 chigia001 marked this pull request as draft July 7, 2023 02:47
@chigia001
Copy link
Contributor Author

chigia001 commented Jul 10, 2023

[email protected] are using nodejs's global performance object, which introduced in node@16. this cause nx fail on node@14
nrwl/nx#17986

PS: this is fixed with [email protected]

@chigia001
Copy link
Contributor Author

Most of the issue is fixed, the issue with browser test will be fix by #1565

@chigia001 chigia001 marked this pull request as ready for review July 11, 2023 08:26
@chigia001 chigia001 marked this pull request as draft July 13, 2023 09:19
@chigia001
Copy link
Contributor Author

I will need some more time to test for tav.

@chigia001 chigia001 closed this Aug 13, 2023
@chigia001 chigia001 deleted the feat-learna-bootstrap-deprecate branch August 13, 2023 05:08
@DanielLoyAugmedix
Copy link

@chigia001 why did this get closed and not merged in?

@chigia001
Copy link
Contributor Author

@DanielLoyAugmedix I have a new pr for this but still WIP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local build differs from CI build Update dependency lerna to v7