Use rollup for more compact build output #81

lelandrichardson · 2017-11-12T04:17:02Z

This PR updates animated to use rollup for a slimmer build.

I'd like to get some feedback on this PR relating to the way we are doing the exports though.

The way I see it, we have 2 options:

Use named exports
This means that import { Value, spring } from 'animated'; would be required, and import Animated from 'animated';would need to beimport * as Animated from 'animated';`. This would be a big breaking change as I think most people import as a single variable binding when using animated.
Use default export
This breaks no one (i think), but i also think that tree shaking certain exports away might not be possible with this approach.

cc @lencioni

lelandrichardson · 2017-11-12T04:17:33Z

cc @browniefed

lelandrichardson · 2017-11-12T04:30:07Z

From what I can tell, this gets the bundle from ~44KB to ~27KB

browniefed · 2017-11-12T17:28:26Z

package.json

@@ -46,6 +46,12 @@
    "babel-preset-react-native": "^1.4.0",
    "browserify": "^13.0.0",
    "react": "^15.4.0",
-    "react-dom": "^15.4.0"
+    "react-dom": "^15.4.0",


Should this be 16 now?

we could, but then the interactive docs will need some refactoring (they use createClass right now). There's nothing about animated that's specific to 15 or 16, so it doesn't need to be changed, but we should move to 16 soon. I want to spend some time integrating animated w/ fiber's scheduling, and so at that time i'll definitely up the dev dep. don't think it should be in this PR though.

Ah gotcha, I'll refactor those and make them not just random inline code

browniefed · 2017-11-12T17:32:14Z

What I'd the size with the default export? Can we release 2 builds somehow, one optimized for named exports and one compatibility build?

lelandrichardson · 2017-11-12T17:33:57Z

@browniefed the 27k mentioned above is with the default export. That includes the whole library. The build with named exports should be basically the exact same size. The difference would be that people not using certain features might be able to use webpack to remove that code.

browniefed · 2017-11-12T21:53:21Z

Ah gotcha, hmm. I wonder if we could do 2 steps.
Can we release this with a codemod to convert to import * as Animated from 'animated';?
Or release first with default export, then major bump with only named exports?

lelandrichardson · 2017-11-12T22:19:07Z

Yeah, I think doing a minor as is and then a Major with named exports might be the way to go. I'm not 100% sold on the named exports though. We should talk to some ppl on the RN team first

…

On Sun, Nov 12, 2017 at 1:53 PM Jason Brown ***@***.***> wrote: Ah gotcha, hmm. I wonder if we could do 2 steps. Can we release this with a codemod to convert to import * as Animated from 'animated';? Or release first with default export, then major bump with only named exports? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#81 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzFty2c36ySLaR2Ob3DTXDbfTERcvWSks5s12jSgaJpZM4QavQL> .

lencioni · 2017-11-13T04:19:28Z

I haven't looked at the code yet, but I'm hesitant about import star. Can you explain why that would be necessary? On Sun, Nov 12, 2017, 2:19 PM Leland Richardson <notifications@github.com> wrote:

…

Yeah, I think doing a minor as is and then a Major with named exports might be the way to go. I'm not 100% sold on the named exports though. We should talk to some ppl on the RN team first On Sun, Nov 12, 2017 at 1:53 PM Jason Brown ***@***.***> wrote: > Ah gotcha, hmm. I wonder if we could do 2 steps. > Can we release this with a codemod to convert to import * as Animated > from 'animated';? > Or release first with default export, then major bump with only named > exports? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#81 (comment)>, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ABzFty2c36ySLaR2Ob3DTXDbfTERcvWSks5s12jSgaJpZM4QavQL > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#81 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAL7zgSSqHrywbw3cf0EpKeVimAECYc_ks5s127cgaJpZM4QavQL> .

lencioni · 2017-11-13T16:41:03Z

.babelrc

+    "syntax-class-properties",
+    "syntax-trailing-function-commas",
+    "transform-class-properties",
+    "transform-es2015-function-name",


I wonder if babel-preset-env can help simplify this config a little?

Or babel-preset-airbnb which uses that :-)

lencioni · 2017-11-13T16:43:47Z

.babelrc

+    "transform-react-jsx",
+    "transform-regenerator",
+    ["transform-es2015-for-of", { "loose": true }],
+    "external-helpers"


It looks like you are bringing in external helpers now, which means that folks will need to import the external helpers file in order for this to continue working for them. This is almost certainly a breaking change, and I'm hesitant to recommend this approach for a library at this time.

Some people are discussing a modular approach to this, so that might become a good option in the future. More info: babel/babel#5699

lencioni · 2017-11-13T16:47:45Z

rollup.config.js

+
+export default [
+    distBuild({ filename: 'animated.umd.js', format: 'umd', sourcemap: true, minify: false }),
+    distBuild({ filename: 'animated.umd.min.js', format: 'umd', sourcemap: true, minify: true }),


Are you already publishing UMD builds? If not, I'm not really sure how much value it adds to start publishing them now, especially if nobody is asking for them. I'd probably leave this out for now.

lencioni · 2017-11-13T16:48:38Z

src/AnimatedValue.js

+import Interpolation from './Interpolation';
+import Animation from './Animation';
+import guid from './guid';
+import SetPilyfill from './SetPolyfill';


typo: Pilyfill

lencioni · 2017-11-13T16:52:54Z

src/AnimatedValue.js

+import guid from './guid';
+import SetPilyfill from './SetPolyfill';
+
+// TODO: wonder if we should do the set polyfill another way...


It would be nice if the Set polyfill from es6-shim was a standalone module that you could just depend on. Alternatively, you could put the burden on consumers and make shimming Set a requirement.

@ljharb might have some thoughts about how to do this best.

I think it’s way simpler for a standard polyfillable feature like Set (or Map, or Promise, etc) to just assume it’s present, and state in the docs that a polyfill is needed.

ljharb

I really think it would be better to start with a normal ESM build, and worry about rollup later.

ljharb · 2017-11-13T17:03:00Z

.babelrc

+    "syntax-class-properties",
+    "syntax-trailing-function-commas",
+    "transform-class-properties",
+    "transform-es2015-function-name",


Or babel-preset-airbnb which uses that :-)

ljharb · 2017-11-13T17:03:38Z

rollup.config.js

+
+export default [
+    distBuild({ filename: 'animated.umd.js', format: 'umd', sourcemap: true, minify: false }),
+    distBuild({ filename: 'animated.umd.min.js', format: 'umd', sourcemap: true, minify: true }),


necolas · 2017-11-15T21:30:01Z

Does this moving the library even further away from the version in RN?

lencioni · 2017-11-30T19:21:54Z

@lelandrichardson is there anything I can do to help move this forward?

Use rollup for more compact build output

cae54bf

lelandrichardson force-pushed the lmr--use-rollup branch from 1e4e8d2 to cae54bf Compare November 12, 2017 04:18

browniefed reviewed Nov 12, 2017

View reviewed changes

lencioni reviewed Nov 13, 2017

View reviewed changes

ljharb reviewed Nov 14, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rollup for more compact build output #81

Use rollup for more compact build output #81

lelandrichardson commented Nov 12, 2017

lelandrichardson commented Nov 12, 2017

lelandrichardson commented Nov 12, 2017

browniefed Nov 12, 2017

lelandrichardson Nov 12, 2017

browniefed Nov 12, 2017

browniefed commented Nov 12, 2017

lelandrichardson commented Nov 12, 2017

browniefed commented Nov 12, 2017

lelandrichardson commented Nov 12, 2017 via email

lencioni commented Nov 13, 2017 via email

lencioni Nov 13, 2017

ljharb Nov 13, 2017

lencioni Nov 13, 2017

lencioni Nov 13, 2017

ljharb Nov 13, 2017

lencioni Nov 13, 2017

lencioni Nov 13, 2017

ljharb Nov 13, 2017

ljharb left a comment

ljharb Nov 13, 2017

ljharb Nov 13, 2017

necolas commented Nov 15, 2017

lencioni commented Nov 30, 2017

Use rollup for more compact build output #81

Are you sure you want to change the base?

Use rollup for more compact build output #81

Conversation

lelandrichardson commented Nov 12, 2017

lelandrichardson commented Nov 12, 2017

lelandrichardson commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

browniefed commented Nov 12, 2017

lelandrichardson commented Nov 12, 2017

browniefed commented Nov 12, 2017

lelandrichardson commented Nov 12, 2017 via email

lencioni commented Nov 13, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necolas commented Nov 15, 2017

lencioni commented Nov 30, 2017