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

Rollup #632

Merged
merged 4 commits into from
Mar 14, 2018
Merged

Rollup #632

merged 4 commits into from
Mar 14, 2018

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Feb 22, 2018

Ref #516

  1. added rollup
  2. replaced DayPicker.dist.js with src/index.umd.js
  3. added all stuff in umd bundle exports (not only Input)
  4. added Input to cjs entry point
  5. ignored lib/src. What is it useful for? I'd even ignore whole lib folder
  6. replaced object-assign package with object spread to use babel helper which is already in the bundle

results

webpack

daypicker.js 103.0K
daypicker.min.js 39.71K

rollup

daypicker.js 59.53K
daypicker.min.js 27.21K

@codecov
Copy link

codecov bot commented Feb 22, 2018

Codecov Report

Merging #632 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #632   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         619    619           
  Branches      134    134           
=====================================
  Hits          619    619

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d16700...c96e7ed. Read the comment docs.

Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

Thank you so much, rollup looks much cleaner and prop-types are stripped altogether 😌

Do you know if will it keep working with unpkg?

I don't remember why we are keeping /lib/src on git. Could it be just for bower? (which will be removed, related #633). lib is kept for style.css

@gpbl gpbl added this to the v8.0.0 milestone Feb 23, 2018
@TrySound
Copy link
Contributor Author

@gpbl I don't think bower is useful for cjs modules. unpkg won't bundle cjs, but can deliver umd from npm. Do you mean style.css is used with brower?

@TrySound
Copy link
Contributor Author

If you want I can commit lib/src, so this PR won't be a breaking change.

@gpbl
Copy link
Owner

gpbl commented Feb 23, 2018

The major version is not much for breaking, just for safety. I won't wait much for the release.

Have you looked at the website from localhost if it works fine?

@TrySound
Copy link
Contributor Author

Not yet

@gpbl
Copy link
Owner

gpbl commented Feb 23, 2018

(not sure if website loads the built version or not, maybe i wrote some docs there?)

@gpbl
Copy link
Owner

gpbl commented Mar 5, 2018

We are quite ready for this, just wait some days to see if user upgrading to v7.1.0 have some unforeseen issues :) Thanks @TrySound for your work!

@gpbl
Copy link
Owner

gpbl commented Mar 13, 2018

Thinking it again, could we keep the Input component outside the DayPicker bundle? not everyone is going to use it. I’m looking for a way to import the two components separately.

@TrySound
Copy link
Contributor Author

Why so? Input component is not so big since you get rid from moment.js. And eventually it even will be treeshaked. I think it's important to keep package solid with single entry point.

@TrySound
Copy link
Contributor Author

I think I can make it treeshakable now too.

@gpbl
Copy link
Owner

gpbl commented Mar 13, 2018

It’s not that big. Still...
We may add more components in the future however. what do you think about supporting also

import DayPicker from “react-day-picker/lib/DayPicker”;

import DayPickerInput from “react-day-picker/lib/DayPickerInput”;

Is too old school? 😊

Does treeshaking work out of the box even with webpack <3?

@TrySound
Copy link
Contributor Author

@gpbl webpack < 4 shouldn't matter, since you gonna release major version. If somebody want to upgrade a few react components they probably will be lucky to upgrade tooling also to get better experience on all sides.

@gpbl
Copy link
Owner

gpbl commented Mar 13, 2018

@gpbl webpack < 4 shouldn't matter, since you gonna release major version.

I don’t see how the two things are related.

If somebody want to upgrade a few react components they probably will be lucky to upgrade tooling also to get better experience on all sides.

Who is going to upgrade to webpack 4? 😅 We are still releasing too many major versions, we must be nice with other devs 😊

@TrySound
Copy link
Contributor Author

TrySound commented Mar 13, 2018

They are related with one command

yarn upgrade-interactive --latest

We introduce breaking change with requirement to use new features which work perfectly, but we do not break code including this component. It will be just a bit bigger. The main problem is that "pathed" imports introduces even more mess over solved issues. Usually projects distribute umd, cjs and esm. UMD can be used for puppeteer tests and via unpkg. cjs is useful for node tests. esm is used by bundlers. If you want to add two entry points you should also duplicate format namespaces.

So your package will have

lib/index.js
lib/Input.js
esm/index.js
esm/Input.js

What do you think users will use first? The wrong one. Unoptimized.

Oh, and UMD will have Input, so entries will be incompatible for end user.

@gpbl
Copy link
Owner

gpbl commented Mar 13, 2018

So, if someone wants to reduce the bundle size should upgrade their tools to use treeshaking. I can agree with this if you say that splitting does complicate stuff too much 👌🏽

@gpbl gpbl changed the base branch from master to v8 March 14, 2018 07:33
@TrySound
Copy link
Contributor Author

I will rebase this today later.

@gpbl gpbl merged commit c96e7ed into gpbl:v8 Mar 14, 2018
@TrySound TrySound deleted the rollup branch March 14, 2018 08:52
@gpbl gpbl removed this from the v8.0.0-beta.1 milestone Feb 21, 2021
@gpbl gpbl added this to the v8.0.0 milestone Feb 21, 2021
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.

2 participants