dividab / tsconfig-paths

Load node modules according to tsconfig paths, in run-time or via API.
MIT License
1.8k stars 100 forks source link

Use an allowlist-based .npmignore file #250

Closed ericcornelissen closed 1 year ago

ericcornelissen commented 1 year ago

I noticed that despite the .npmignore file of this project, files from the __tests__/ directory are still being published:

Proof ![npm code preview for the `tsconfig-paths` package](https://github.com/dividab/tsconfig-paths/assets/3742559/6aecf847-ce03-4a68-9b58-0954fba7b45c)

After looking at #137 and #186, and reading For the love of god, don’t use .npmignore as well as the npm docs "Developer Guide", I came to the following conclusion: Using both "files" and .npmignore does not(/no longer?) work, and since allowlisting(/whitelisting) is being advocated in that article I implemented an allowlist-based .npmignore file.

The .npmignore file I created simply ignores everything (using /*), then un-ignores the patterns previously specified in package.json#files (using !xyz), and then re-ignores the patterns previously in the .npmignore.

An alternative to the approach I took would be to increase the specificity of file patterns in the "files" array to avoid including test files (and removing the .npmignore file).

Manual Tests & Motivating Examples I tested various approaches using `npm publish --dry-run` to find out what worked and what didn't. For starters, on 11b774d994b897c6c8e87dda57375a285813731d (the latest commit on `master` as of writing): ```sh $ npm publish --dry-run npm notice npm notice 📦 tsconfig-paths@4.2.0 npm notice === Tarball Contents === npm notice 13.7kB CHANGELOG.md npm notice 1.1kB LICENSE npm notice 10.0kB README.md npm notice 2.3kB package.json npm notice 26B register.js npm notice 3.1kB src/__tests__/config-loader.test.ts npm notice 8.0kB src/__tests__/data/match-path-data.ts npm notice 1.6kB src/__tests__/filesystem.test.ts npm notice 1.2kB src/__tests__/mapping-entry.test.ts npm notice 702B src/__tests__/match-path-async.test.ts npm notice 580B src/__tests__/match-path-sync.test.ts npm notice 3.9kB src/__tests__/try-path.test.ts npm notice 12.3kB src/__tests__/tsconfig-loader.test.ts npm notice 184B src/__tests__/tsconfig-named.json npm notice 2.3kB src/config-loader.ts npm notice 2.4kB src/filesystem.ts npm notice 522B src/index.ts npm notice 2.0kB src/mapping-entry.ts npm notice 6.0kB src/match-path-async.ts npm notice 4.7kB src/match-path-sync.ts npm notice 3.2kB src/register.ts npm notice 3.3kB src/try-path.ts npm notice 6.0kB src/tsconfig-loader.ts npm notice === Tarball Details === npm notice name: tsconfig-paths npm notice version: 4.2.0 npm notice filename: tsconfig-paths-4.2.0.tgz npm notice package size: 20.0 kB npm notice unpacked size: 89.0 kB npm notice shasum: eca129f62afa81df6b82792f84529b7a36f1c9d7 npm notice integrity: sha512-qczxCjVV+2J5f[...]ZbZcz2KHD6wVQ== npm notice total files: 23 npm notice npm notice Publishing to https://registry.npmjs.org/ with tag latest and default access (dry-run) + tsconfig-paths@4.2.0 ``` includes the test files as "expected". To verify the `.npmignore` file doesn't do anything I changed it as: ```diff __tests__/ __stories__/ *.test.{js,ts,tsx} + CHANGELOG.md ``` doing that gives me: ```sh $ npm publish --dry-run npm notice npm notice 📦 tsconfig-paths@4.2.0 npm notice === Tarball Contents === npm notice 13.7kB CHANGELOG.md npm notice 1.1kB LICENSE npm notice 10.0kB README.md npm notice 2.3kB package.json npm notice 26B register.js npm notice 3.1kB src/__tests__/config-loader.test.ts npm notice 8.0kB src/__tests__/data/match-path-data.ts npm notice 1.6kB src/__tests__/filesystem.test.ts npm notice 1.2kB src/__tests__/mapping-entry.test.ts npm notice 702B src/__tests__/match-path-async.test.ts npm notice 580B src/__tests__/match-path-sync.test.ts npm notice 3.9kB src/__tests__/try-path.test.ts npm notice 12.3kB src/__tests__/tsconfig-loader.test.ts npm notice 184B src/__tests__/tsconfig-named.json npm notice 2.3kB src/config-loader.ts npm notice 2.4kB src/filesystem.ts npm notice 522B src/index.ts npm notice 2.0kB src/mapping-entry.ts npm notice 6.0kB src/match-path-async.ts npm notice 4.7kB src/match-path-sync.ts npm notice 3.2kB src/register.ts npm notice 3.3kB src/try-path.ts npm notice 6.0kB src/tsconfig-loader.ts npm notice === Tarball Details === npm notice name: tsconfig-paths npm notice version: 4.2.0 npm notice filename: tsconfig-paths-4.2.0.tgz npm notice package size: 20.0 kB npm notice unpacked size: 89.0 kB npm notice shasum: eca129f62afa81df6b82792f84529b7a36f1c9d7 npm notice integrity: sha512-qczxCjVV+2J5f[...]ZbZcz2KHD6wVQ== npm notice total files: 23 npm notice npm notice Publishing to https://registry.npmjs.org/ with tag latest and default access (dry-run) + tsconfig-paths@4.2.0 ``` still including `CHANGELOG.md`... Next I converted `.npmignore` to an allowlist and added the patterns from `package.json#files`: ```diff + /* + + !/src + !/lib + !register.js + !package.json + !CHANGELOG.md + !LICENSE + !README.md + __tests__/ __stories__/ *.test.{js,ts,tsx} ``` which results in: ```sh $ npm publish --dry-run npm notice npm notice 📦 tsconfig-paths@4.2.0 npm notice === Tarball Contents === npm notice 13.7kB CHANGELOG.md npm notice 1.1kB LICENSE npm notice 10.0kB README.md npm notice 2.1kB package.json npm notice 26B register.js npm notice 8.0kB src/__tests__/data/match-path-data.ts npm notice 184B src/__tests__/tsconfig-named.json npm notice 2.3kB src/config-loader.ts npm notice 2.4kB src/filesystem.ts npm notice 522B src/index.ts npm notice 2.0kB src/mapping-entry.ts npm notice 6.0kB src/match-path-async.ts npm notice 4.7kB src/match-path-sync.ts npm notice 3.2kB src/register.ts npm notice 3.3kB src/try-path.ts npm notice 6.0kB src/tsconfig-loader.ts npm notice === Tarball Details === npm notice name: tsconfig-paths npm notice version: 4.2.0 npm notice filename: tsconfig-paths-4.2.0.tgz npm notice package size: 16.2 kB npm notice unpacked size: 65.6 kB npm notice shasum: 643fd2ac6ccabb26da3aba2d9b3b32cf8f85e12e npm notice integrity: sha512-09nHu94GMCgfb[...]x4FTYhqMHjaEg== npm notice total files: 16 npm notice npm notice Publishing to https://registry.npmjs.org/ with tag latest and default access (dry-run) + tsconfig-paths@4.2.0 ``` which only ignored the `*.test.{js,ts,tsx}` files... Hence, I prefixed the `__tests__/` and `__stories__/` patterns with `**/`

As an aside, it looks like the **/__stories__/ and *.test.{js,ts,tsx} patterns are unnecessary for this project. If you want they can be removed as part of this Pull Request as well :slightly_smiling_face:

jonaskello commented 1 year ago

Thanks for working through this! I see that you have also tested it locally so I'm OK to merge this :-)