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

fix(node): add optional peer dependencies for the default plugins #1853

Closed
wants to merge 1 commit into from

Conversation

dobesv
Copy link
Contributor

@dobesv dobesv commented Jan 20, 2021

Yarn PnP requires all dependencies loaded at runtime using require() to
be declared. Dependencies that are "optional" like these plugins can be
declared as a "peerDependency" that is optional.

By adding the default plugins as peer dependencies we should get this to work with yarn pnp out of the box, at least for those plugins.

Yarn PnP requires all dependencies loaded at runtime using `require()` to
be declared.  Dependencies that are "optional" like these plugins can be
declared as a "peerDependency" that is optional.
@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #1853 (6cfb0d3) into master (2fcb76d) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1853      +/-   ##
==========================================
- Coverage   92.63%   92.61%   -0.02%     
==========================================
  Files         174      174              
  Lines        6054     6054              
  Branches     1288     1288              
==========================================
- Hits         5608     5607       -1     
- Misses        446      447       +1     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@dyladan
Copy link
Member

dyladan commented Jan 20, 2021

Wow I didn't know about the being able to make these optional. Does it then suppress warnings typically emitted if peer dependencies are missing?

@dobesv
Copy link
Contributor Author

dobesv commented Jan 20, 2021

@obecny
Copy link
Member

obecny commented Jan 20, 2021

ok but why you are trying to add this to node package ?

@dyladan
Copy link
Member

dyladan commented Jan 20, 2021

ok but why you are trying to add this to node package ?

When the PluginLoader tries to require("@opentelemetry/plugin-something") but @opentelemetry/plugin-something is not in dependencies, Yarn PnP complains. This fixes that. The plugin loader is in node package so that is where the dependency fix needs to be applied.

@dobesv
Copy link
Contributor Author

dobesv commented Jan 20, 2021

The dependencies have to be listed on the package that calls require()

@obecny
Copy link
Member

obecny commented Jan 20, 2021

please hold with this, very soon node package will not have PluginLoader anymore -> #1760

@dobesv dobesv closed this Jan 20, 2021
@dobesv dobesv deleted the plugin-peer-deps branch January 20, 2021 20:47
@dyladan
Copy link
Member

dyladan commented Feb 3, 2021

I think this is a good idea while we wait on #1760 which will take a while to implement.

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.

3 participants