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(package.json): publish source maps #570

Merged
merged 2 commits into from Jul 17, 2021

Conversation

blumamir
Copy link
Member

Which problem is this PR solving?

  • publish source maps files to npm. This is already enabled in some of the packages in this repo (but missing in others), and was added to core repo a year ago:

Short description of the changes

  • add "build/src/**/*.js.map" to "files" section in package.json

@blumamir blumamir requested a review from a team as a code owner July 13, 2021 12:46
@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #570 (38cc493) into main (1ba3ce1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #570   +/-   ##
=======================================
  Coverage   94.78%   94.78%           
=======================================
  Files         179      179           
  Lines       10976    10976           
  Branches     1088     1088           
=======================================
  Hits        10404    10404           
  Misses        572      572           

@Flarna
Copy link
Member

Flarna commented Jul 13, 2021

Shouldn't we include the ts files also?
At least VsCode complains that it can't find ts files if there are source map files pointing to "nowhere".

@blumamir
Copy link
Member Author

Shouldn't we include the ts files also?
At least VsCode complains that it can't find ts files if there are source map files pointing to "nowhere".

Don't really know,
I fix this due to warnings about missing source map files in one of our client's IDE, which was solved by adding the js.map files to npm publish.
I remember it was missing in core and then added in the PR I mentioned above. Then we added it to the instrumentations in ext-js here and the warning went away for these packages.
The packages from the contrib still generate warnings about missing source map files which I believe will also be solved if we include the ".js.map" in the distribution.

@MSNev
Copy link
Contributor

MSNev commented Jul 13, 2021

Shouldn't we include the ts files also?

If we change the sourceMap in the tsconfig.json to inlineSourceMap then no as the generated sourcemap will then contain the original source. While this makes the *.map files huge its a lot easier to handle as it includes the original code.

I'm not sure about any other packaging switches / tools that would also need additional config changes to retain these (like webpack for example).

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@obecny obecny added the enhancement New feature or request label Jul 15, 2021
@vmarchaud vmarchaud merged commit be5be06 into open-telemetry:main Jul 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants