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

feat: update webpack outside of examples #963

Merged
merged 4 commits into from
Apr 12, 2022

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented Apr 8, 2022

Which problem is this PR solving?

Webpack hasn't been updated for a while. Testing out pnpm, I noticed webpack@4 doesn't work with it at all, so out of curiousity(the research of pnpm is still ongoing) had to hack it to work.

I'm not familiar with configuring webpack at all so any feedback to the changes are very welcome. The build results are identical.

Short description of the changes

  • Updated all webpack- and karma-related packages.
  • Changed karma.conf.js to include all plugins form the relative package.json - Karma failed autoloading all deps from the package.json if the deps were symlinked.
  • Installed node polyfills for webpack, it used to include those, but doesn't in webpack@5.

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable. Browser-based packages do not use TAV

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #963 (6cff55e) into main (80e855a) will decrease coverage by 0.99%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #963      +/-   ##
==========================================
- Coverage   95.91%   94.91%   -1.00%     
==========================================
  Files          13       24      +11     
  Lines         856     1180     +324     
  Branches      178      236      +58     
==========================================
+ Hits          821     1120     +299     
- Misses         35       60      +25     
Impacted Files Coverage Δ
...ction/src/contentScript/InstrumentationInjector.ts 100.00% <0.00%> (ø)
...rc/background/ProgrammaticContentScriptInjector.ts 100.00% <0.00%> (ø)
...njection/src/instrumentation/WebInstrumentation.ts 97.22% <0.00%> (ø)
...-instrumentation-aws-lambda/src/instrumentation.ts 90.72% <0.00%> (ø)
...emetry-id-generator-aws-xray/src/platform/index.ts 100.00% <0.00%> (ø)
...extension-autoinjection/src/contentScript/index.ts 0.00% <0.00%> (ø)
...r-aws-xray/src/platform/node/AWSXRayIdGenerator.ts 100.00% <0.00%> (ø)
...y-id-generator-aws-xray/src/platform/node/index.ts 100.00% <0.00%> (ø)
...metry-browser-extension-autoinjection/src/types.ts 90.32% <0.00%> (ø)
...er-extension-autoinjection/src/background/index.ts 0.00% <0.00%> (ø)
... and 1 more

@rauno56 rauno56 marked this pull request as ready for review April 8, 2022 09:20
@rauno56 rauno56 requested a review from a team April 8, 2022 09:20
@Flarna
Copy link
Member

Flarna commented Apr 8, 2022

I'm surpriesed that you got this running. A while ago I tried something similar in core repo but failed (created open-telemetry/opentelemetry-js#2187 that time).

One of the reasons why I failed were that there are dependencies which seem to require webpack for, e.g. istanbul-instrumenter-loader has "webpack": "^2.0.0 || ^3.0.0 || ^4.0.0" in its peerDependencies.

@rauno56
Copy link
Member Author

rauno56 commented Apr 8, 2022

Those errors are ignored if legacy peer deps is turned on, so I guess that's why it hasn't popped onto my radar.

  • Must check whether code coverage reporting still works. Thanks for the reference.

@rauno56
Copy link
Member Author

rauno56 commented Apr 9, 2022

Even though instanbul-instrumenter-loader worked with webpack@5, the peer dep range did not include v5. Replaced it with another package that's otherwise identical but doesn't have that problem.

@rauno56
Copy link
Member Author

rauno56 commented Apr 11, 2022

Any additional comments/questions, @Flarna?

@Flarna
Copy link
Member

Flarna commented Apr 11, 2022

No questions. Nice that this works.
As I have not really an idea of these browser packages/setups I'm not sure if I'm the right one to approve here.

@dyladan
Copy link
Member

dyladan commented Apr 11, 2022

No questions. Nice that this works. As I have not really an idea of these browser packages/setups I'm not sure if I'm the right one to approve here.

I think nobody is left who remembers when they were set up

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Maybe we can apply this to the main repo too?

@Flarna
Copy link
Member

Flarna commented Apr 11, 2022

No questions. Nice that this works. As I have not really an idea of these browser packages/setups I'm not sure if I'm the right one to approve here.

I think nobody is left who remembers when they were set up

I meant there are maybe others in reviewers/maintainers group who have a better understanding of browser tooling. If not browser support will most likely degrade more and more over time.

@rauno56
Copy link
Member Author

rauno56 commented Apr 12, 2022

Looks good. Maybe we can apply this to the main repo too?

Just linked this PR to a relevant issue for now, but I'll keep that in mind.

@rauno56 rauno56 merged commit 9a58648 into open-telemetry:main Apr 12, 2022
@rauno56 rauno56 deleted the chore/update-webpack branch April 12, 2022 09:47
@dyladan dyladan mentioned this pull request Apr 12, 2022
MSNev added a commit to MSNev/opentelemetry-js-contrib that referenced this pull request Aug 30, 2022
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.

7 participants