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

chore: add typescript declarations to .gitignore #2665

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

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented Dec 19, 2024

When npm-link-ing Endo against external projects, the types are not resolvable without first building the declaration files (unlike how types are resolved within the project itself). This requires compiling declaration files in the appropriate Endo workspaces and re-running as needed.

Previously, all generated declaration files (and their sourcemaps) were recognized as unstaged new files by Git, which is incredibly annoying when trying to see what changes have been made to Endo sources.

This change adds declaration files, their sourcemaps and any build cache files (*.tsbuildinfo) to Git's ignorelist.

Escape Hatch

Sometimes it's helpful to keep declarations under version control. To that end, any new declaration file that needs to be tracked should match:

/^.+\.types\.d\.[mc]?ts$/

Example: foo.types.d.ts. This will avoid the need to force-add the file to Git.

Notes

  • To avoid the need to rename files (e.g., the various types.js files amongst the workspaces), this pattern was not chosen:
    /^types.*\.d\.[mc]?ts$/
  • If the need arises to track a directory full of declaration files, another convention can be established and appended to the ignorefile.
  • All clean scripts will need to be updated, since git clean needs another flag (-X) to delete ignored files.

What I'm Dealing With

An example of my workflow is something like this:

npm link -w @endo/compartment-mapper \
  -w ses \
  -w @endo/env-options \
  -w @endo/cjs-module-analyzer \
  -w @endo/static-module-record \
  -w @endo/zip \
  -w @endo/evasive-transform
cd /path/to/lavamoat
npm link @endo/compartment-mapper \
  ses \
  @endo/env-options \
  @endo/cjs-module-analyzer \
  @endo/static-module-record \
  @endo/zip \
  @endo/evasive-transform

At this point, my build will fail because the type declarations I need are missing. So I need to:

cd /path/to/endo
npm run prepack -w @endo/compartment-mapper -w ses

This will generate the declarations I need, but git status results in:

...
?? packages/compartment-mapper/src/policy.d.ts
?? packages/compartment-mapper/src/policy.d.ts.map
?? packages/compartment-mapper/src/powers.d.ts
?? packages/compartment-mapper/src/powers.d.ts.map
?? packages/compartment-mapper/src/search.d.ts
?? packages/compartment-mapper/src/search.d.ts.map
?? packages/compartment-mapper/src/types/compartment-map-schema.d.ts
?? packages/compartment-mapper/src/types/compartment-map-schema.d.ts.map
?? packages/compartment-mapper/src/types/external.d.ts
?? packages/compartment-mapper/src/types/external.d.ts.map
?? packages/compartment-mapper/src/types/internal.d.ts
?? packages/compartment-mapper/src/types/internal.d.ts.map
?? packages/compartment-mapper/src/types/node-powers.d.ts
?? packages/compartment-mapper/src/types/node-powers.d.ts.map
?? packages/compartment-mapper/src/types/policy-schema.d.ts
?? packages/compartment-mapper/src/types/policy-schema.d.ts.map
?? packages/compartment-mapper/src/types/policy.d.ts
?? packages/compartment-mapper/src/types/policy.d.ts.map
?? packages/compartment-mapper/src/types/powers.d.ts
?? packages/compartment-mapper/src/types/powers.d.ts.map
?? packages/compartment-mapper/src/types/typescript.d.ts
?? packages/compartment-mapper/src/types/typescript.d.ts.map
?? packages/compartment-mapper/src/url.d.ts
?? packages/compartment-mapper/src/url.d.ts.map
?? packages/compartment-mapper/tsconfig.build.tsbuildinfo

...which is annoying if I have actual changes in Endo!

Script changes

I updated clean and postpack scripts en masse:

  • Now that declaration files are ignored, git needs the -X flag to delete them
  • Normalized all postpack scripts:
    • Updated with new patterns matching newly-ignored declaration, declaration maps, and .tsbuildinfo files
    • If a workspace has a clean script which was essentially identical to postpack, postpack just invokes yarn clean
    • Replace single quotes ' with escaped double quotes \" for the sake of portability (it is ugly, but Endo may want to support a Windows-based development environment one day)
    • ses, in particular, has a clean script which differs substantially from postpack, so I left clean as-is.

No change to packed files

I used my preferred diff tool to compare new-pack.json, generated by this PR:

npm pack -ws --json --dry-run > new-pack.json

...and old-pack.json, generated by the master branch:

npm pack -ws --json --dry-run > old-pack.json

The only differences were the package.json files having different sizes and hashes due to the changes to the clean and postpack scripts (as noted above)


Description updated Jan 17 2025

@boneskull boneskull self-assigned this Dec 19, 2024
@boneskull
Copy link
Contributor Author

(Yes, this change is basically just for me)

@boneskull boneskull added the devex developer experience label Dec 19, 2024
Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a lot of manually authored .d.ts files so a blanket ignore of them is not ok. In general I wish we had a CI job to verify that any files in the repo are not currently excluded by the gitignore config, as it's a pain to manage files "force added"

@mhofman
Copy link
Contributor

mhofman commented Dec 19, 2024

  • When publishing, npm's default behavior is to skip ignored files unless they are present in a package.json's files array. I believe this is already configured correctly, but future packages must maintain the same strategy. I recommend carefully examining the output of npm pack --dry-run before the next publish to avoid regressions in the list of published artifacts.

I believe @kriskowal has some commands to verify no regression here that I'd like to see the output of before approving a merge.
Basically list all the files in packed packages before and after, and diff the lists.

@boneskull
Copy link
Contributor Author

@mhofman I feel like the concern may be overblown.

git ls-files '*.d.ts'
packages/bundle-source/src/exports.d.ts
packages/captp/src/ts-types.d.ts
packages/cli/test/_types.d.ts
packages/compartment-mapper/src/types-external.d.ts
packages/compartment-mapper/src/types.d.ts
packages/daemon/src/types.d.ts
packages/daemon/types.d.ts
packages/eventual-send/src/exports.d.ts
packages/eventual-send/src/types.d.ts
packages/exo/src/types.d.ts
packages/far/src/exports.d.ts
packages/lp32/types.d.ts
packages/pass-style/src/types.d.ts
packages/ses/src/reporting-types.d.ts
packages/ses/types.d.ts
packages/stream/types.d.ts
packages/trampoline/types.d.ts
packages/where/types.d.ts

We aren't really adding a ton of these on a regular basis.

@boneskull
Copy link
Contributor Author

This should surface everything to be published:

npm pack --workspaces --json --dry-run

So we can diff the output of that

@turadg
Copy link
Member

turadg commented Dec 19, 2024

One way forward is to have a consistent module name for hand-written types and make that an exclusion in gitignore.

@boneskull
Copy link
Contributor Author

boneskull commented Dec 19, 2024

Some tests are failing because git clean -f '*.d.ts' doesn't work anymore. 😄

tsc --clean should be a workable replacement in most cases.

UPDATE: git clean -fX is what we want now

@boneskull
Copy link
Contributor Author

boneskull commented Dec 19, 2024

@turadg Since I was touching the postpack scripts, I noticed that some of them are git clean -f '*.d.ts*' '*.tsbuildinfo' and others are git clean -f '*.d.ts*'; should all of them be the former?

@mhofman
Copy link
Contributor

mhofman commented Dec 19, 2024

As @turadg mentioned, while we may not have a lot of these manual definition files currently, I would really like them to be excluded one way or another from being ignored. Force added files are a pain to deal with.

.gitignore Outdated Show resolved Hide resolved
@turadg turadg removed their request for review January 6, 2025 20:03
@boneskull boneskull force-pushed the boneskull/ignore-declarations branch from 0a6e879 to de6eba7 Compare January 17, 2025 22:13
@boneskull
Copy link
Contributor Author

@turadg @kriskowal @mhofman This is ready for re-review; please see the updated PR description

When `npm`-`link`-ing Endo against external projects, the types are not resolvable without first building the declaration files (unlike how types are resolved within the project itself).  This requires compiling declaration files in the appropriate Endo workspaces and re-running as needed.

Previously, all generated declaration files (and their sourcemaps) were recognized as unstaged new files by Git, which is incredibly annoying when trying to see what changes have been made to Endo sources.

**This change adds declaration files, their sourcemaps and any build cache files (`*.tsbuildinfo`) to Git's ignorelist.**

## Escape Hatch

Sometimes it's helpful to keep declarations under version control.  To that end, any _new_ declaration file that needs to be tracked should match `^.+\.types\.d\.[mc]?ts$`. Example: `foo.types.d.ts`. This will avoid the need to force-add the file to Git.

## Notes

- To avoid the need to rename files (e.g., the various `types.js` files amongst the workspaces), the pattern `^types.*\.d\.[mc]?ts$` was **not** chosen.
- If the need arises to track a directory full of declaration files, another convention can be established and appended to the ignorefile.
- All `clean` scripts will need to be updated, since `git clean` needs another flag (`-X`) to delete _ignored_ files.
- Now that declaration files are ignored, `git` needs the `-X` flag to delete them
- Normalized all `postpack` scripts:
  - Updated with new patterns matching newly-ignored declaration, declaration maps, and `.tsbuildinfo` files
  - If a workspace has a `clean` script which was essentially identical to `postpack`, `postpack` just invokes `yarn clean`
  - Replace single quotes `'` with escaped double quotes `\"` for the sake of portability (it is ugly, but Endo may want to support a Windows-based development environment one day)
  - `ses`, in particular, has a `clean` script which differs substantially from `postpack`, so I left `clean` as-is.
@boneskull boneskull force-pushed the boneskull/ignore-declarations branch from de6eba7 to 7e39b49 Compare January 17, 2025 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devex developer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants