feat: add esnext target for auto-instr-web package #1848
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1848 +/- ##
=======================================
Coverage 91.45% 91.45%
=======================================
Files 144 144
Lines 7406 7406
Branches 1483 1483
=======================================
Hits 6773 6773
Misses 633 633 |
| "build/esm/**/*.js.map", | ||
| "build/esm/**/*.d.ts", | ||
| "build/esnext/**/*.js", | ||
| "build/esnext/**/*.js.map", |
There was a problem hiding this comment.
This shouldn't block this PR, but I was comparing to https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/web/opentelemetry-instrumentation-document-load/package.json#L36-L46 and I noticed it uses .../*.map instead of .../*.js.map.
% find build -name "*.map"
build/test/documentLoad.test.js.map
build/test/index-webpack.js.map
build/test/index-webpack.d.ts.map
build/test/documentLoad.test.d.ts.map
build/esm/types.js.map
...
There are also *.d.ts.map files. I wonder if those files are superfluous for end-users so restricting to just *.js.map as is done here is better (just for having a smaller npm install size).
There was a problem hiding this comment.
To be honest I'm not actually sure why we include source maps the way we do.
End users may create source maps for when they bundle their code, as it helps bring them from unreadable bundled code back to their source. I assume we have them to help map from the built JavaScript code back to the original TypeScript code.
I started walking through this with @opentelemetry/instrumentation-document-load. The maps indicate the TypeScript source file which we don't actually include in the published build. The instrumentation.js.map file for example shows "sources":["../../src/instrumentation.ts"], which doesn't actually exist in the build. If we included the TypeScript source file in the published build, then it would be useful, but otherwise I don't think it actually does anything.
As an example: If I am using new DocumentLoadInstrumentation() in my app, and in my IDE I click to Go to Definition, it brings me to instrumentation.d.ts. If I include src in the files for the published build and then reinstall, then clicking on new DocumentLoadInstrumentation() in my app brings me to a instrumentation.ts package.
I'm not sure if we want to include the whole TypeScript source in a published build (and obviously that's not something to decide either way on this PR), but without it, I actually don't think any of these maps are necessary at all 🤔 . Happy to be pointed out where I might be missing something though!
| "compilerOptions": { | ||
| "rootDir": "src", | ||
| "outDir": "build/esnext", | ||
| "jsx": "react", |
There was a problem hiding this comment.
Is the "jsx" option needed for this package? It is specified for plugins/web/opentelemetry-plugin-react-load/tsconfig.* but not for other packages.
There was a problem hiding this comment.
Hmm probably not, I'll remove that.
There was a problem hiding this comment.
I've removed the jsx option since there are no .tsx files in this package and added skipLibCheck to skip type checking of declaration files, matching the tsconfig for esm.
Which problem is this PR solving?
Short description of the changes
esnextbuild target for theauto-instrumentations-webmetapackageesmbuild to"files"along with the newly builtesnextbuilds.