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: exclude tests from published package #840

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kyletsang
Copy link

Current versions of v11 are including the tests in the published package.

This PR fixes that by preventing transpilation of the test folder in the root tsconfig and uses a test-specific tsconfig which does transpile test files. The tsconfig.test.json only targets esm since the original test command uses that folder only. Declarations are not emitted because they are not used.

@broofa
Copy link
Member

broofa commented Nov 21, 2024

Hey, thanks for catching this. I agree removing the test files probably makes sense. (That said I vaguely recall a conversation with someone a few years back making the case for including them...? But I don't recall when / where and I can't make a good case for keeping them.)

Instead of having a separate build pipeline for the test files - which has a whiff of code smell to it - let's add an .npmignore file with the necessary incantation(s) for excluding the test dir(s).

@kyletsang
Copy link
Author

Hey @broofa, I looked over this today with the npmignore and ran into this from the docs:

At the root of your package it will not override the "files" field, but in subdirectories it will.

I wasn't able to get the npmignore to exclude the test dir. Another approach would be to switch out the "dist" entry in files with a glob like "dist/{cjs,cjs-browser,esm,esm-browser}/*.{js,d.ts}".

Thoughts?

@broofa
Copy link
Member

broofa commented Nov 23, 2024

Oof, sorry. 'Didn't realize that .npmignore was so ... ineffective. 😞

It turns out glob negations work in the files field so let's please do the following, instead:

  1. Revert the current changes.
  2. Change package.json#files to be as follows:
  "files": [
    "dist",
    "!dist/**/test"
  ]

And, yes, remove the *.md entries that are currently there. npm includes README and LICENSE by default, and the CHANGELOG and CONTRIBUTING docs don't need to be in the published package. This'll cut the package size from ~1.2M -> 0.8M.

BTW, npm pack --dry-run will show you which files will be published. (In case you weren't aware of that.) I've tested the change above and it works for me, but please verify and let me know if you see any issues.

@kyletsang
Copy link
Author

Confirmed on my end, everything is as expected. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants