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

upgrade rollup and move to maintained plugins #23

Merged
merged 2 commits into from
Feb 6, 2020
Merged

Conversation

manzt
Copy link
Collaborator

@manzt manzt commented Feb 4, 2020

I upgraded rollup to the latest version and migrated rollup plugins to their maintained versions.

rollup-plugin-node-resolve --> @rollup/plugin-node-resolve
rollup-plugin-commonjs --> @rollup/plugin-commonjs
rollup-plugin-json --> @rollup/plugin-json

Currently @rollup/plugin-typescript doesn't support generating .d.ts files but will likely soon. Might be worth reevaluating if that happens since rollup-plugin-typescript2 is apparently quite slow.

@manzt manzt requested a review from gzuidhof February 4, 2020 01:08
@manzt
Copy link
Collaborator Author

manzt commented Feb 4, 2020

FWIW: rollup-plugin-typescript2 is faster than @wessberg/rollup-plugin-ts currently (which also generates type info). I tried this locally, and the current set up is best bet right now IMO. ~2.5 vs 5.0 seconds.

@wessberg
Copy link

wessberg commented Feb 5, 2020

@MantZ, thanks for trying out rollup-plugin-ts. I'd like to just chime in here and state that the extended compilation time when generating declarations is due to rollup-plugin-ts bundling and tree-shaking declaration files as described here, something that neither rollup-plugin-typescript2, @rollup/plugin-typescript or even tsc does. As with any bundling step, that process is expensive and could explain the additional build time. I would be surprised if raw compilation times without declaration bundling differ from a cold cache. That said, there are of course many potential ways the declaration bundling can be optimized.

@manzt
Copy link
Collaborator Author

manzt commented Feb 5, 2020

I'm a bit out of my depth here with rollup in general, but I very much appreciate the detailed response @wessberg! Perhaps @gzuidhof will have some input, but I will definitely do some more exploring. I think evaluating which typescript plugin best supports our needs is beyond the scope of this PR, but something we should certainly consider more thoroughly.

Copy link
Owner

@gzuidhof gzuidhof left a comment

Choose a reason for hiding this comment

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

Thanks for updating the deps Trevor :).

As for treeshaking, I don't think it's very relevant for this library (we don't really have many dependencies), it's probably something worth considering for apps that actually use this library in the real world.

@manzt manzt merged commit 0906c76 into master Feb 6, 2020
@manzt manzt deleted the manzt/rollup-upgrade branch February 11, 2020 23:05
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.

3 participants