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

build: rollup port of #977 #984

Merged

Conversation

GerkinDev
Copy link
Contributor

@GerkinDev GerkinDev commented Mar 15, 2023

This PR also includes some missing dependencies install & configuration, rollup returned warnings.

Build size results:

File esbuild output rollup output (old) rollup output (new)
react-tooltip.cjs.js 31K 106K 35K
react-tooltip.cjs.min.js 13K 36K 12K
react-tooltip.css 1.7K 1.6K 1.6K
react-tooltip.d.ts 4.3K 4.3K 4.3K
react-tooltip.esm.js 28K 105K 34K
react-tooltip.esm.min.js 11K 35K 11K
react-tooltip.iife.js 32K X X
react-tooltip.iife.min.js 13K X X
react-tooltip.umd.js X 112K 39K
react-tooltip.umd.min.js X 36K 12K
react-tooltip.min.js 11K X X
react-tooltip-tokens.css 201 201 201
react-tooltip.min.css X 1.3K 1.3K

Rollup minified outputs are actually smaller.

Note: source maps are not generated by rollup, should they ?

  • yes

@danielbarion
Copy link
Member

hey @GerkinDev, thanks for this PR!

Also, can you include the exports from my PR, please?

I'll test this guy as soon as I have time, if this one works better than esbuild for distribution files, we'll merge this one and close my PR :)

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Mar 15, 2023

I got the feeling that the rollup config can be reworked to perform only one or 2 build(s) to several outputs, instead of 6 like currently. Do you want me to try it, or maybe in a later PR ?

rollup.config.prod.js Outdated Show resolved Hide resolved
@danielbarion
Copy link
Member

hey @GerkinDev I'm sorry for the delay, I'm very busy with my job in the past few days, I'll take a look at this PR as soon as I have free time.

thank you so much for this PR!

@GerkinDev
Copy link
Contributor Author

No problem @danielbarion . Yet, would you like me to do a deeper rework of rollup config? I can find a bit of time for this if you think it's relevant.

@danielbarion
Copy link
Member

danielbarion commented Mar 19, 2023

@GerkinDev yeah, go ahead with the rollup config (what do you have in mind? 👀 ), also, I believe the source map will help the community with the debugging, so we can enable it too

@danielbarion
Copy link
Member

yarn add react-tooltip@5.10.1-beta-10

or

npm install react-tooltip@5.10.1-beta-10

Copy link
Member

@danielbarion danielbarion left a comment

Choose a reason for hiding this comment

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

hey @GerkinDev I'll test as soon as possible, today I released a beta version so everyone can test too.

thanks!

also, can you update the PR to generate the source map too, please?

@GerkinDev
Copy link
Contributor Author

Hi Daniel, no problem for sourcemaps & other things, I just didn't had the time since I'm currently away for work. But I did not forget, don't worry

@danielbarion
Copy link
Member

danielbarion commented Mar 27, 2023

@GerkinDev can you update this branch with this commit, please?

691b8b6

Also, I released this beta version:

yarn add react-tooltip@5.10.2-beta.1

or

npm install react-tooltip@5.10.2-beta.1

That works on a Next 13 project using the experimental app folder and swc - dev and production build.

cc @gabrieljablonski

@danielbarion
Copy link
Member

danielbarion commented Mar 27, 2023

@GerkinDev can you check the test that is broken in node 16, please?

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Mar 27, 2023

Sry I was experimenting stuff to get rid of react/jsx-runtime, that was hard to use with CDNs in UMD formats.

FYI pure CDN setup would look like this (without centralizing CDNs):

    <script crossorigin src="https://unpkg.com/react@18/umd/react.development.js"></script>
    <script crossorigin src="https://unpkg.com/react-dom@18/umd/react-dom.development.js"></script>
    <script src="https://cdn.jsdelivr.net/npm/@floating-ui/core@1.2.5"></script>
    <script src="https://cdn.jsdelivr.net/npm/@floating-ui/dom@1.2.5"></script>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/classnames/2.3.2/index.min.js" integrity="sha512-GqhSAi+WYQlHmNWiE4TQsVa7HVKctQMdgUMA+1RogjxOPdv9Kj59/no5BEvJgpvuMTYw2JRQu/szumfVXdowag==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>
    <script crossorigin src="<your-cdn.com>/react-tooltip.umd.min.js"></script>
    <link rel="stylesheet" href="<your-cdn.com>/react-tooltip.min.css"></link>

I've found a way to make the example-v5 work to check this out.

Rollup config has been fully reworked. Bundles are just a bit (a few bytes) bigger than the original version, but all should work now.

@danielbarion
Copy link
Member

Oh, thanks but the example-v5 was just the initial docs before we create the current one with docusaurus, when we develop new feature or bug fix, we use the app-dev to test, and after that, we check if our docs continue to work as expected :)

I'll check the rollup build this week, thanks!

@GerkinDev
Copy link
Contributor Author

I just wanted to find a way to test build outputs somehow, and I did not knew which documentation to use.

I think I'm pretty much done here. Some global setup were affected, but you should not need to do anything different when working on this. In the worst case, tooling should clearly tell you what is missing: you just now need to import React from 'react' when using jsx syntax.
Feel free to do any feedbacks ! Cheers ! 🍻

@danielbarion
Copy link
Member

oh, I see!

new beta release with the latest commit of this branch:

yarn add react-tooltip@5.10.2-beta.4

or

npm install react-tooltip@5.10.2-beta.4

@gabrieljablonski
Copy link
Member

Everything seems ok, we'll just have to go back on the open issues and check which are solved with this PR.

Copy link
Member

@danielbarion danielbarion left a comment

Choose a reason for hiding this comment

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

All good from my side too, thanks for letting us know @gabrieljablonski

@danielbarion
Copy link
Member

@gabrieljablonski @GerkinDev I believe this PR is good to be merged, I just tested an example from one of the open issues and it's still breaking, but when upgrading Next.js to 13.2.5-canary.22 release, the issue goes away, so we can merge this one as it's a great improvement to the project and lets wait the next release of Next.js to check if the issues are solved.

Can you guys please let me know if you agree or not with 👍🏻 👎🏻 ?

Thanks!

@gabrieljablonski
Copy link
Member

Fine by me. There are still a few other open issues that are related, but I'm fine with just revisiting them after the merge.

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Mar 31, 2023

Since issues existed before this PR, there is no real regression. Worst case scenario I see is that issues are not properly fixed, but that might be up to next.js team and not you guys. So as far as I'm concerned, LGTM.

Thanks for the time took for in-situ checks @danielbarion . Don't hesitate to poke me if you need anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants