-
Notifications
You must be signed in to change notification settings - Fork 7
build: migrate bundle pipeline to tsdown
#305
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
base: main
Are you sure you want to change the base?
Conversation
c71f768
to
c44e1fb
Compare
Issues are fixed & tests run locally as expected 👍🏻 (cc @serhalp) |
sorry for the delays on this one @TheAlexLichter. If you can give this one more update we'll get it merged! Thanks for the PR! 😄 |
@serhalp No worries! Updated, build + tests passing locally. (Lint errors are from main) |
@TheAlexLichter hmm, those tests are failing on all platforms and I can repro locally on your branch. how are you running tests locally? build first: npm run --workspaces build run all tests: npm run --workspaces test:ci (or run tests for one package (e.g. npm run -w ./packages/vite-plugin test:ci seems to be something with a transitive dependency of |
hmm @TheAlexLichter:
from https://tsdown.dev/guide/getting-started these packages mostly support and test is this PR even viable, given this? |
|
||
export default defineConfig([ | ||
{ | ||
clean: true, | ||
format: ['cjs', 'esm'], | ||
entry: ['src/bootstrap/main.ts', 'src/main.ts'], | ||
tsconfig: 'tsconfig.json', | ||
splitting: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe related to rolldown/tsdown#317 ? @TheAlexLichter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine as consumers import via the entry points only 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we turned off splitting because of a packaging issue in deno 2 that we should probably investigate if we're changing that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's ensure that this packaging issue is resolved by now (or we can adapt the config to avoid code splitting with a workaround)
when we test on node 18, we build on node 18: https://github.com/netlify/primitives/blob/main/.github/workflows/test.yaml#L56-L90 (we don't have to do that, but we do that at the moment) |
Checked locally and building + testing works fine with Node 18 too 👍🏻 ![]() |
As tsup is not actively maintained anymore and there are faster tools as drop-in replacement available (based on Rolldown), I took a few minutes to migrate the packages over to
tsdown
.This should yield in faster builds (bundling and speedy DTS generation).
PS: Sorry for the drive-by PR!