chore: npm ignore several files used for dev

[tlhunter]

I noticed that there are many files included in the npm package that aren't necessary at run time, even from when Datadog was still doing releases.

Definitely open to conversation about these changes.

.editorconfig -> required for local development
.eslintrc.yaml -> required for local development
.release-please-manifest.json -> required for CI
release-please-config.json -> required for CI
tsconfig.json -> required for local development only? Is this used by library consumers?
test -> required for local development, CI
# CHANGELOG.md -> could be useful for a user who is debugging stuff
# CODE_OF_CONDUCT.md -> a contributor would clone the repo so should be safe to ignore too?
# CONTRIBUTING.md -> a contributor would clone the repo so should be safe to ignore too?
# GOVERNANCE.md -> a contributor would clone the repo so should be safe to ignore too?

1.8.1

1.9.0

[timfish]

Thanks for the PR.

These changes to files inlcuded in packaging look good to me!

[trentm]

@tlhunter Another option would be to use the "files" property of package.json to explicitly allow-list the files to include. For example, with:

diff --git a/package.json b/package.json
index 45d4a06..a8ae4f2 100644
--- a/package.json
+++ b/package.json
@@ -23,6 +23,12 @@
     "hook",
     "hooks"
   ],
+  "files": [
+    "lib",
+    "*.js",
+    "*.mjs",
+    "*.d.ts"
+  ],
   "author": "Bryan English <bryan.english@datadoghq.com>",
   "license": "Apache-2.0",
   "bugs": {

the published files will be:

% npm pack --dry-run
npm notice
npm notice 📦  import-in-the-middle@1.9.0
npm notice Tarball Contents
npm notice 11.4kB LICENSE
npm notice 2.7kB README.md
npm notice 11.7kB hook.js
npm notice 412B hook.mjs
npm notice 3.6kB index.d.ts
npm notice 2.4kB index.js
npm notice 3.4kB lib/get-esm-exports.js
npm notice 3.9kB lib/get-exports.js
npm notice 1.1kB lib/register.js
npm notice 2.1kB package.json
npm notice Tarball Details
npm notice name: import-in-the-middle
npm notice version: 1.9.0
npm notice filename: import-in-the-middle-1.9.0.tgz
npm notice package size: 13.7 kB
npm notice unpacked size: 42.7 kB
npm notice shasum: d208e61a7115f24cbbb0d17de6950585c8108212
npm notice integrity: sha512-Ml5G/dvFueyDk[...]uMwAO64SviLjw==
npm notice total files: 10
npm notice
import-in-the-middle-1.9.0.tgz

npm pack (and hence npm publish) always include the package.json, README, and LICENSE files.

[trentm]

Another option ...

If "files" is considered, then we'd have the same discussion about what files to publish that aren't strictly needed for runtime. My bias would be to include:

• the LICENSE* and NOTICE files
• the CHANGELOG.md file (That's a personal preference. I occasionally like to look at these when poking around in node_modules/... dirs.)

Nothing else.

tsconfig.json -> required for local development only? Is this used by library consumers?

This is only required for the tests, AFAICT -- for the ./test/typescript/ts-node.test.mts test.

[trentm]

That said (my comments about the "files" option), this is definitely an improvement. Thanks.

[timfish]

I'd be against using files unless we change all of the tests to actually test against a generated package from npm pack.

It would be very easy to add source files and forget to update files and publish a broken release!

[timfish]

Although now I'm looking again I see the wildcard/glob paths for files and this feels less of an issue.

.npmignore excluding files has always felt safer than specifically including files but it depends 🤷‍♂️

[Qard]

I think the test files may need to be present for citgm to work. 🤔

[trentm]

It would be very easy to add source files and forget to update files and publish a broken release!

Before the days of ubiquitous CI and publishing via CI, and the first time I npm publishd and accidentally published a whole swath of local tmp dev files ... I was converted to explicit "files". :)

I think either is fine.

[trentm]

I think the test files may need to be present for citgm to work. 🤔

@Qard Do you know of where, if anywhere, something is using citgm on IITM currently? IIUC citgm could be used against a git tag tarball, which would then include the test files if needed.

[Qard]

It's in the lookup config here: https://github.com/nodejs/citgm/blob/main/lib/lookup.json#L214-L218

Presumably that means it's using npm install?

[trentm]

It's in the lookup config here: https://github.com/nodejs/citgm/blob/main/lib/lookup.json#L214-L218

Hrm. Looking at a few other packages that are listed in that config file: none of glob, express, got, gulp publish their test files.

[trentm]

From a perhaps naive read of citgm, my guess is that:

• citgm-all (https://github.com/nodejs/citgm?tab=readme-ov-file#citgm-all) is being run in Node.js' CI (Jenkins). I can't tell because there isn't a Jenkinsfile that I can see and the CI Jenkins is under security embargo right now.
• citgm-all tests packages from a GitHub tarball by default, unless "npm": true in taht lookup.json... of which the only such package is "minimist".

So, I think dropping tests from the published IITM will be fine and if not, the fix in citgm will be trivial.

[jsumners-nr]

Maintaining .npmignore and (package.json).files is prone to error:

fastify/skeleton#42 (comment)

[Qard]

I'm fine just landing this as-is and if CITGM breaks we can revert. Seems like it should probably be fine, but I'm not 100% confident of that.

Maintaining .npmignore and (package.json).files is prone to error

I think it should be expected those need to be maintained separately. They have two entirely different targets. 🤷🏻