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: removing publishConfigs, using .d.ts for types #1055

Merged
merged 47 commits into from
Jun 28, 2023

Conversation

arboleya
Copy link
Member

@arboleya arboleya commented Jun 9, 2023

TL;DR

This PR favors .d.ts files (over .ts ones) as type definitions, even for local development. This requires rebuilding the workspace to see changes but increases reliability and compatibility with other tools.

  • Upgrade tsup
  • Refactor all tsup configs
  • Add postbuilld script (code)
  • Refactor all package.json configs
  • Stops using tsup for generating types
  • Start using tsc for generating types w/ Declaration Maps
  • Create a new app in internal directory for validating configs

Note This problem arose when I was working on #1049, which is blocked until this is merged.

Summary

The attempt to use .ts files instead of .d.ts for development (introduced by #984), didn't work as expected. VSCode only fully worked for main entry points (not secondary ones), and other apps/bundlers would malfunction locally—probably due to the format discrepancies between DTS and TS.

Because of this, I'm bringing back the use of Declaration Maps files (d.ts.map), from my first experiments in #827. Although it adds an extra step, it is more reliable and compatible with other tools. (sorry @camsjams, I tried a lot)

This approach offers

  1. Single build (not more publishConfig overrides)
    • Local packages are 100% identical to published ones!
  2. Increased reliability (local builds works like published ones)
  3. Increased compatibility with other tools (NextJS, CRA, Vite)

BUT

  1. We need to build the repo to see the changes
    • Command pnpm dev automates this
    • TurboRepo cache helps a bit with speed
    • We can further reduce the over-fragmentation of packages in the future

Final Notes

If you can, please review this other PR before this one:

I separated that into another PR to make it easier to review this PR without the bulkiness of the demo apps, but I will need to merge that PR into this one before sending it to master.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2023

This PR is published in NPM with version 0.0.0-pr-1055-20230619152516

Torres-ssf
Torres-ssf previously approved these changes Jun 20, 2023
luizstacio
luizstacio previously approved these changes Jun 27, 2023
Copy link
Member

@luizstacio luizstacio left a comment

Choose a reason for hiding this comment

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

Lgtm

Dhaiwat10
Dhaiwat10 previously approved these changes Jun 27, 2023
internal/check-imports/src/imports.ts Outdated Show resolved Hide resolved
danielbate
danielbate previously approved these changes Jun 28, 2023
Torres-ssf
Torres-ssf previously approved these changes Jun 28, 2023
Copy link
Contributor

@Torres-ssf Torres-ssf left a comment

Choose a reason for hiding this comment

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

🚀

LuizAsFight
LuizAsFight previously approved these changes Jun 28, 2023
@arboleya arboleya dismissed stale reviews from LuizAsFight and Torres-ssf via af8ec90 June 28, 2023 11:51
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

LGTM

@arboleya arboleya merged commit cd47596 into master Jun 28, 2023
6 checks passed
@arboleya arboleya deleted the aa/chore/removing-publish-configs branch June 28, 2023 12:07
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.

7 participants