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(draft): switch to Rolldown #11

Closed
wants to merge 8 commits into from
Closed

Conversation

AdrianGonz97
Copy link
Member

@AdrianGonz97 AdrianGonz97 commented Aug 22, 2024

This PR replaces Rollup for Rolldown. Currently a WIP, but it's in a functional state (locally).

Notes

  • Rolldown doesn't seem to know how to resolve import paths that use .js extensions to reference a TS file. Using a .ts extension seems to work instead, but for now, I just removed all extensions
  • Packages that are written in CJS needed to be externalized as Rolldown doesn't transform require calls into ESM imports at this time (tracking: [Feature Request]: Support transforming CJS require calls into ESM imports rolldown/rolldown#2041)
  • Dynamic import vars aren't fully supported yet, so they've been replaced for normal dynamic imports with static paths

TODO

  • Rolldown doesn't have a built-in watcher yet, so we'll have to create our own for the time being (tracking: [Feature Request]: watch mode rolldown/rolldown#2028)
  • Investigate why Rolldown is transforming dedent into malformed code (it's being externalized for now)
  • Move ast-manipulation and ast-tooling directly into core

Copy link

pkg-pr-new bot commented Aug 22, 2024

commit: 94f27b5

pnpm add https://pkg.pr.new/sveltejs/cli/sv@11
pnpm add https://pkg.pr.new/sveltejs/cli/@svelte-cli/core@11

Open in Stackblitz

@dominikg
Copy link
Member

would not recommend using extensionless imports.

why switch to rolldown now, is it solving an issue you have with rollup?

@AdrianGonz97
Copy link
Member Author

let me preface by saying that this pr isn't a guarantee or commitment that we'll be adding rolldown. we're just exploring to see if we can get it to work and if it's a good fit

would not recommend using extensionless imports.

i very much agree, i'm just trying to get things to work atm. i'm also not in a rush in converting every import path to using a .ts extension until we're sure we're going to move forward with this pr

why switch to rolldown now, is it solving an issue you have with rollup?

a few reasons:

  • significantly faster builds (we're going from ~7 seconds to ~150ms to build the whole project, including dts files)
  • greatly simplifies the build tools
  • allows us to combine closely related packages into core. since builds are so much faster, we no longer have to perform our hack of having a dedicated package just for the ast-tooling to avoid long build times in dev. it worked for rollup, but it's also very annoying and inconvenient to work with. with rolldown, we can avoid this issue altogether

@manuel3108
Copy link
Member

manuel3108 commented Aug 23, 2024

Amazing! Just tried this out locally, and it's indeed much faster. But at least for me the timing is not as good as on your end. I'm going down from about 8 seconds to 1.6 seconds, while spending about 850ms on ast-tooling.

I find it interesting that there are so drastic differences between build times. Looks like ci is also taking a bit longer than a second. Are your times potentially based on svelte-add repo? Having another look at the ci logs, i would assume that our create package is taking this long. Although there is no way to be sure, as they don't print the dirs in the output sadly.

Edit: Not sure if im blind, but it really is not printing the package name / directory it's currently working on. Couldnt find an issue in their repo either.

@dominikg
Copy link
Member

for me it goes from 4s to 0.9s for pnpm build in the root.
Regarding the ast-tooling that gets bundled, how large do you estimate the sv tgz is going to be? If the plan is that users always call pnpm dlx sv keeping that small/fast is important.

An alternative could be to allow local installation in devDependencies so it profits from shared deps.

@dominikg
Copy link
Member

however i have some reservations against using rolldown to build sv while it is still under rapid prerelease development. Unless we have tests that ensure the output is always correct and we don't accidentally release a version of sv that is broken due to a change in rolldown, this would be very risky

@benmccann
Copy link
Member

I clearly have the slowest computer 😆 Mine went from 16.6s to 3.7s. Though a majority of that was from:

execSync('node scripts/build-templates.js', { cwd: path.resolve('packages', 'create') });

Perhaps we could just dynamically import that script rather than creating a child process?

@AdrianGonz97
Copy link
Member Author

AdrianGonz97 commented Aug 23, 2024

Are your times potentially based on svelte-add repo?

ah yeah, the original times were based off the svelte-add repo, which didn't include the new dts plugin. basing it off of this repo's main branch, the time is ~3.5 seconds with rollup

But at least for me the timing is not as good as on your end. I'm going down from about 8 seconds to 1.6 seconds, while spending about 850ms on ast-tooling.

in both cases, i excluded the build-cli-templates plugin since that adds anywhere from 400-900ms (it varies by a lot), and i'm also excluding pnpm by running rolldown/rollup's bin directly since startup times with pnpm can add ~300-600ms

Perhaps we could just dynamically import that script rather than creating a child process?

calling it directly would definitely be ideal, though we'll need to hoist up create's devDeps to the root so they're accessible to the rolldown config.

edit: or we could expose the scripts by adding a new export condition to the create package (like ./scripts) and call the build scripts that way

@AdrianGonz97
Copy link
Member Author

this PR is starting to have too many conflicts to handle, which is fine. in the future, if/when we decide to switch to Rolldown, it'll be nice to have this as a reference

@hyf0
Copy link

hyf0 commented Dec 8, 2024

Happen to saw this PR. Happy to share some updates and thoughts.

Rolldown doesn't seem to know how to resolve import paths that use .js extensions to reference a TS file.

This is supported in nightly version.

Packages that are written in CJS needed to be externalized as Rolldown doesn't transform require calls into ESM imports at this time

This is solved too. We now polyfill require if the format is cjs and platform is node.

Dynamic import vars aren't fully supported yet, so they've been replaced for normal dynamic imports with static paths

We have a builtin plugin to do it. But not sure if it fits this use case.

https://github.com/rolldown/rolldown/blob/de64ad96e4c827cf0908e64ca879a9c8837c9869/packages/rolldown/src/experimental-index.ts#L9

Rolldown doesn't have a built-in watcher

Yeah. Watcher is supported.

however i have some reservations against using rolldown to build sv while it is still under rapid prerelease development. Unless we have tests that ensure the output is always correct and we don't accidentally release a version of sv that is broken due to a change in rolldown, this would be very risky

@dominikg

Yeah. This is a very legit worry. We have done some process to ensure correctness, such as that rolldown is currently built by itself for every commit and run all tests.

We are planning a 1.0.0 beta release this month and certainly we would have a ecosystem CI to run before publishing any new version.

The ecosystem CI will clone the repo that uses rolldown, run the build command and test command.

Not intend to pushing anything. But if you decide to switched to rolldown, we surely will add this repo as a part of ecosystem CI.

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.

5 participants