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

refactor: move types into types.ts #47

Merged
merged 4 commits into from
Jan 1, 2024

Conversation

psibean
Copy link
Contributor

@psibean psibean commented Dec 30, 2023

This PR should not have any API impact, it is purely a typescript refactor which maintains existing build output and behaviour.

  • Move all type exports from index.ts to types.ts
  • Add tsup as a dev dependency for build:types script - this allows a single type declaration file to be output from multiple typescript files
  • tsup now programmatically generates both the index.d.ts and index.d.cts files - it's no longer a direct copy
  • Type declarations aren't needed in the github workflow, tsup currently only supports Node >= 18, so this was breaking the workflow. Workflow still builds the code and also runs the tests ensuring 14, 16, 18 compatibility
  • Added Node 20 to the workflow as it's now an LTS.

I compared the new type declaration output to the old / previous and it appears to me to be the same - if someone else could confirm / verify that would be nice!

Since we have a breaking change to fix a current bug / missing feature, I also plan on renaming some of the types to be less generic to avoid potential type name clashing in the global scope (alternatively, wrap declared types in a "csrf-csrf" module block).

@psibean psibean force-pushed the refactor/move-types-to-their-own-file branch from 473229a to 4d689c0 Compare December 30, 2023 10:15
@psibean psibean self-assigned this Dec 30, 2023
Copy link
Collaborator

@davidgonmar davidgonmar left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, but the build command seems to leave an 'empty' types.js file (both on the esm and cjs builds), which shouldn't cause any problems but probably isn't intended. After looking for a while it seems it is the way tsc works. Also, if types are being explicitly declared in the types.ts file, shouldn't it be named types.d.ts or something similar?

@psibean
Copy link
Contributor Author

psibean commented Jan 1, 2024

Looks pretty good to me, but the build command seems to leave an 'empty' types.js file (both on the esm and cjs builds), which shouldn't cause any problems but probably isn't intended. After looking for a while it seems it is the way tsc works. Also, if types are being explicitly declared in the types.ts file, shouldn't it be named types.d.ts or something similar?

The compilation will still output it as a declaration file. I don't want a manual declaration file to be maintained, the ts file will still have type checking on it where a hard coded declaration wouldn't.

The point of moving them is just to separate the code a bit - make it easier to maintain / review and move the type only things out of the implementation.

The second one here being the most important, it's a side effect of tsc trying to support certain build tools. Seems to be up in the air whether they'll provide an option to prevent export {} / empty module generation.

Lots of people complaining that type only exports / imports end up with this side effect.

Using tsup to also transpile the ts implementation seems to give a different output compared to tsc, so I don't think I want to make that switch yet, although it would prevent the empty output.

For now I just added some manual removals of the empty types.js

davidgonmar
davidgonmar previously approved these changes Jan 1, 2024
@psibean psibean merged commit 14c1f0e into main Jan 1, 2024
7 checks passed
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