dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

Use files property in package.json instead of .npmignore

Open watson opened this issue 8 months ago • 5 comments
trafficstars

What does this PR do?

Delete .npmignore and use the files property in package.json to select the files to include in the published npm package.

Motivation

The n/no-unpublished-require ESLint rule incorrectly flagged some require calls from the root of our project into the packages directory as referencing files not published to npm. This was a bug in that rule.

After researching how we select files to be published or not, I found that we use .npmignore as an allowlist instead of a denylist - which it is normally meant to be used as. Thinking this might somehow trigger the bug, I deleted the .npmignore file and moved the allowlist to the files property in package.json and that fixed the problem.

Plugin Checklist

Additional Notes

I've verified the package produced by npm pack between this branch and master and the the only difference, is that the package produced on this branch doesn't include packages/memwatch/package.json. However, I think that was a mistake to include in the first place, so I'd say this change is actually an improvement.

watson avatar Feb 26 '25 09:02 watson

This stack of pull requests is managed by Graphite. Learn more about stacking.

watson avatar Feb 26 '25 09:02 watson

Overall package size

Self size: 9.61 MB Deduped: 108.63 MB No deduping: 109.01 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 9.0.0 | 19.6 MB | 19.61 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.9.0 | 9.77 MB | 10.14 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.3 | 2.95 MB | 5.6 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 10.3.0 | 617.18 kB | 1.08 MB | | import-in-the-middle | 1.14.2 | 122.36 kB | 850.93 kB | | lru-cache | 10.4.3 | 804.3 kB | 804.3 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | limiter | 3.0.0 | 157.92 kB | 157.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 7.0.5 | 63.38 kB | 63.38 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.9 | 25.11 kB | 25.11 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.3 | 23.74 kB | 23.74 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.4 | 3.96 kB | 3.96 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

github-actions[bot] avatar Feb 26 '25 09:02 github-actions[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.85%. Comparing base (d4c55ba) to head (3ed3382). Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5320      +/-   ##
==========================================
+ Coverage   81.78%   81.85%   +0.07%     
==========================================
  Files         471      472       +1     
  Lines       19310    19431     +121     
==========================================
+ Hits        15792    15906     +114     
- Misses       3518     3525       +7     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 26 '25 09:02 codecov[bot]

Datadog Report

Branch report: watson/npmignore2 Commit report: d9bed6f Test service: dd-trace-js-integration-tests

:x: 138 Failed (138 Known Flaky), 1015 Passed, 0 Skipped, 20m 49.45s Total Time

:x: Failed Tests (138)

This report shows up to 5 failed tests.

  • jest CommonJS can report git metadata - integration-tests/jest/jest.spec.js - :snowflake: Known flaky - Details

    Expand for error
    imeout
    
  • jest CommonJS can report git metadata - integration-tests/jest/jest.spec.js - :snowflake: Known flaky - Details

    Expand for error
    imeout
    
  • jest CommonJS can run and report tests with agentless - integration-tests/jest/jest.spec.js - :snowflake: Known flaky - Details

    Expand for error
    imeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/dd-trace-js/dd-trace-js/integration-tests/jest/jest.spec.js)
    
  • jest CommonJS can run and report tests with agentless - integration-tests/jest/jest.spec.js - :snowflake: Known flaky - Details

    Expand for error
    imeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/dd-trace-js/dd-trace-js/integration-tests/jest/jest.spec.js)
    
  • jest CommonJS can run and report tests with evp proxy - integration-tests/jest/jest.spec.js - :snowflake: Known flaky - Details

    Expand for error
    imeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/dd-trace-js/dd-trace-js/integration-tests/jest/jest.spec.js)
    

Benchmarks

Benchmark execution time: 2025-07-09 10:27:04

Comparing candidate commit 3ed3382023a54a122e260d20926ab71754abaa4e in PR branch watson/npmignore2 with baseline commit d4c55ba45235ccfe9e6735d5deb8b8025efbb813 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1165 metrics, 48 unstable metrics.

pr-commenter[bot] avatar Feb 26 '25 09:02 pr-commenter[bot]

Looks pretty good, always better to have whitelist than blacklist. Are we sure the memwatch thingy should be excluded ? Another thing i'm thinking is, if we forget to include files in the package, we might not realize before it's already released, since I think our CI passes tests on the repo code, not the package code right ?

simon-id avatar Jul 08 '25 18:07 simon-id

@simon-id Please read the PR description ;) All your questions are already addressed there, but the TLDR is:

Are we sure the memwatch thingy should be excluded ?

Pretty sure it was a bug to include it before

Another thing i'm thinking is, if we forget to include files in the package, we might not realize before it's already released, since I think our CI passes tests on the repo code, not the package code right ?

You're right, but this PR doesn't change that issue, since we always has used the .npmignore file as if it was a files array in pacakge.json.

You can argue that we should make a CI check for this, but that's unrelated to this PR.

watson avatar Jul 09 '25 04:07 watson

Yeah I read the description already, but how can we be sure it was a bug in the first place you know ? i don't even know what that memwatch thing does :shrug: And as for the second point you're right, i got confused by the double negation, npmignore+negative asserts == same thing as files array yes :+1:

simon-id avatar Jul 09 '25 06:07 simon-id

I added a commit which includes all files automatically added by npm, which technically doesn't have to be included in the files list. But based on review feedback it has been decided to be explicit as to not rely on knowledge of what npm includes to know what will be included.

watson avatar Jul 09 '25 09:07 watson