Upgrading deps #446

alexreardon · 2018-04-16T23:24:14Z

No description provided.

alexreardon · 2018-04-16T23:25:33Z

.size-snapshot.json

@@ -1,21 +1,21 @@
 {
  "dist/react-beautiful-dnd.js": {
-    "bundled": 372617,


Main code change i can see is our moving from throw new Error to invariant in an attempt to reduce kb's in tree shaking. However, it looks like our umd bundle went up. Any thoughts @TrySound ?

Don't know to be honest.

@lukastaegert what was changed in the latest versions?

I updated the snapshot on master so we are sure we are looking at the correct thing

I had a look at the build and it is not creating multiple copies of invariant..

I think it might be a transitive dependency change..

Okay, i reversed the invariant usages changes and i can confirm that the size changes did not come from there

alexreardon · 2018-04-16T23:26:07Z

src/types.js

@@ -364,15 +364,15 @@ export type DropState = {|
  result: ?DropResult,
 |}

-export type State = {
+export type State = {|


now an exact type

…o upgrading-deps

alexreardon · 2018-04-16T23:50:12Z

.size-snapshot.json

-    "gzipped": 35556
+    "bundled": 360109,
+    "minified": 132835,
+    "gzipped": 37619
  },
  "dist/react-beautiful-dnd.esm.js": {


our esm builds are looking better for moving to invariant rather than throwing, but otherwise we took a huge hit!

What is the reason for using invariant over throw statements? Did you notice any effect on Flow's ability to narrow the types on things?

Here is the reason for using invariant:

* The invariant message will be stripped in production, but the invariant * will remain to ensure logic does not differ in production.

Flow understands that invariant will throw if the condition is not met

alexreardon · 2018-04-16T23:59:52Z

package.json

    "raf-stub": "^2.0.2",
    "react": "^16.3.1",
    "react-dom": "^16.3.1",
    "react-test-renderer": "^16.3.1",
    "rimraf": "^2.6.2",
-    "rollup": "^0.56.2",
+    "rollup": "^0.58.0",


could this be the cause?

alexreardon · 2018-04-17T00:12:10Z

yarn.lock

-[email protected], babylon@^7.0.0-beta.40:
-  version "7.0.0-beta.42"
-  resolved "https://registry.yarnpkg.com/babylon/-/babylon-7.0.0-beta.42.tgz#67cfabcd4f3ec82999d29031ccdea89d0ba99657"
+[email protected]:


This might also be the cause

alexreardon · 2018-04-17T00:12:21Z

yarn.lock

-rollup@^0.56.2:
-  version "0.56.5"
-  resolved "https://registry.yarnpkg.com/rollup/-/rollup-0.56.5.tgz#40fe3cf0cd1659d469baad11f4d5b6336c14ce84"
+rollup@^0.58.0:


this could have caused it

nah i just checked

alexreardon · 2018-04-17T02:37:29Z

Given that the regression is only for the UMD bundle i am tentatively okay to proceed.

We cannot reasonably stay on old deps to avoid the kb hit.

Although i do think it very worthwhile to investigate the cause.

Any takers @lukebatchelor @TrySound ?

petegleeson · 2018-04-17T03:09:50Z

package.json

    "babel-cli": "^6.26.0",
-    "babel-eslint": "^8.2.1",
+    "babel-core": "^6.26.0",


No babel-core before this?

it was being brought in by another dep. now we are being more explicit

petegleeson · 2018-04-17T03:13:54Z

src/state/dimension.js

@@ -134,10 +134,11 @@ export const scrollDroppable = (
    clipped,
  };

-  return ({
+  const result: DroppableDimension = {


Does flow complain without this cast?

nope! it now works with exact types and spread

petegleeson · 2018-04-17T03:17:19Z

src/state/reducer.js

@@ -266,13 +266,13 @@ export default (state: State = clean('IDLE'), action: Action): State => {
        return existing;
      }

-      const newDrag: DragState = ({
+      const newDrag: DragState = {


Could clean up the Flow annotations by typing this IIFE like:

const drag = ((): ?DragState => { // ... })();

lukebatchelor · 2018-04-17T03:30:28Z

So, looking at the diff of the two bundles (before and after this PR) I get an interesting result.

Immediately I see the new bundle has invariant bundled at the top and in each place the old bundle has a null check and a throw, the new bundle has a call to invariant.

Strangely, further down I do then see a copy of invariant in the old bundle though (one that is not included in the new bundle)... So.. You would assume these would cancel each other out no?

Also, all of invariant including the license header is 1.4k, a lot less that the 38k difference we are seeing reported?

Overall, just looking at the diff, if anything I would actually expect the old bundle to be bigger... I'll show you the diff when you're around.

lukebatchelor · 2018-04-17T03:50:28Z

Oh, also worth noting, the actual sizes I'm getting don't match the output above

$ cat original.js | wc -c
372696

$ cat new.js | wc -c
372551

$ echo "372696 - 372551" | bc
145

Only 145 bytes difference.

alexreardon · 2018-04-17T04:21:16Z

package.json

@@ -44,7 +44,6 @@
  },
  "dependencies": {
    "babel-runtime": "^6.26.0",
-    "invariant": "^2.2.4",


I wrote my own. I will put it on npm soon.

alexreardon · 2018-04-17T04:21:28Z

package.json

@@ -44,7 +44,6 @@
  },
  "dependencies": {
    "babel-runtime": "^6.26.0",
-    "invariant": "^2.2.4",


I wrote my own. I will put it on npm soon.

alexreardon · 2018-04-17T04:22:03Z

src/invariant.js

@@ -0,0 +1,21 @@
+// @flow
+
+const isProduction: boolean = process.env.NODE_ENV === 'production';


why complicate things? a super simple invariant function that actually works

I have tested the bundles and the messages are now being stripped correctly

alexreardon · 2018-04-17T05:55:05Z

package.json

    "memoize-one": "^3.1.1",
    "prop-types": "^15.6.0",
    "raf-schd": "^2.1.1",
    "react-motion": "^0.5.2",
    "react-redux": "^5.0.6",
    "redux": "^3.7.2",
    "redux-thunk": "^2.2.0",
-    "reselect": "^3.0.1"
+    "reselect": "^3.0.1",
+    "tiny-invariant": "^0.0.3"


right now i have not figured out how to get our rollup build to tree shake the env conditional insight of this module. However, i am happy to solve that later

alexreardon · 2018-04-17T06:39:17Z

I have no idea where the extra 27,156 bytes came from... I had a look at the diff and i cannot see how we got such an increase

Original

cat dist/react-beautiful-dnd.js | wc -c
  372696

New

cat dist/react-beautiful-dnd.js | wc -c
  399852

alexreardon · 2018-04-17T06:55:35Z

Given that it will only impact the umd distribution i am okay to proceed at this point. However, it would be worth getting to the bottom of this one

alexreardon · 2018-04-17T06:57:23Z

This could be relevant:

./src/index.js → dist/react-beautiful-dnd.js...

Computed sizes of "dist/react-beautiful-dnd.js" with "umd" format
  bundled: 399,839 B
  minified with uglify: 149,239 B
  minified and gzipped: 42,623 B


(!) Missing exports
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
commonjs-proxy:/Users/areardon/code/react-beautiful-dnd/node_modules/core-js/library/modules/es6.object.to-string.js
default is not exported by node_modules/core-js/library/modules/es6.object.to-string.js
1: import * as es6_object_toString from "/Users/areardon/code/react-beautiful-dnd/node_modules/core-js/library/modules/es6.object.to-string.js"; export default ( es6_object_toString && es6_object_toString['default'] ) || es6_object_toString;

TrySound · 2018-04-17T09:03:10Z

Seems like the problem with size and missing exports is related to new rollup version and core-js.

TrySound · 2018-04-17T09:44:02Z

Yep, it's regression in rollup

alexreardon · 2018-04-17T10:44:56Z

Thanks @TrySound. I will merge this PR as is and we can create a follow up issue to fix the umd build

alexreardon added 2 commits April 17, 2018 09:02

removing flow opt outs for spread. moving to invariant for kb savings

1fdd5d7

updating snapshot

9a1f2af

alexreardon requested a review from petegleeson April 16, 2018 23:24

alexreardon commented Apr 16, 2018

View reviewed changes

fixing broken test

994e463

alexreardon commented Apr 16, 2018

View reviewed changes

alexreardon added 2 commits April 17, 2018 09:46

Merge branch 'master' of github.com:atlassian/react-beautiful-dnd int…

7ed54a4

…o upgrading-deps

fixing yucky merge

35aa95c

alexreardon commented Apr 16, 2018

View reviewed changes

alexreardon commented Apr 17, 2018

View reviewed changes

adding babel-core as it is currently an unmet peer dep

532f589

alexreardon requested a review from treshugart April 17, 2018 02:44

petegleeson approved these changes Apr 17, 2018

View reviewed changes

moving from invariant to something that actually works

d6495e2

alexreardon commented Apr 17, 2018

View reviewed changes

alexreardon added 3 commits April 17, 2018 15:40

swapping to use tiny-invariant

75ab753

updating snapshot

496a762

setting production env first for umd build

54e8695

alexreardon commented Apr 17, 2018

View reviewed changes

bumping more deps

8aaa7f3

TrySound mentioned this pull request Apr 17, 2018

Commonjs with core-js treeshakability regression rollup/rollup#2130

Closed

alexreardon mentioned this pull request Apr 17, 2018

Fix rollup UMD bundle size regression #447

Closed

alexreardon merged commit 82a387d into master Apr 17, 2018

alexreardon deleted the upgrading-deps branch April 17, 2018 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrading deps #446

Upgrading deps #446

alexreardon commented Apr 16, 2018

alexreardon Apr 16, 2018

TrySound Apr 16, 2018

alexreardon Apr 16, 2018

alexreardon Apr 16, 2018

alexreardon Apr 16, 2018

alexreardon Apr 16, 2018

alexreardon Apr 16, 2018

alexreardon Apr 16, 2018

petegleeson Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon Apr 16, 2018

alexreardon Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon commented Apr 17, 2018

petegleeson Apr 17, 2018

alexreardon Apr 17, 2018

petegleeson Apr 17, 2018

alexreardon Apr 17, 2018

petegleeson Apr 17, 2018

lukebatchelor commented Apr 17, 2018

lukebatchelor commented Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon Apr 17, 2018

alexreardon commented Apr 17, 2018

alexreardon commented Apr 17, 2018

alexreardon commented Apr 17, 2018

TrySound commented Apr 17, 2018

TrySound commented Apr 17, 2018

alexreardon commented Apr 17, 2018

		@@ -0,0 +1,21 @@
		// @flow

		const isProduction: boolean = process.env.NODE_ENV === 'production';

Upgrading deps #446

Upgrading deps #446

Conversation

alexreardon commented Apr 16, 2018

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

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

alexreardon commented Apr 17, 2018

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

lukebatchelor commented Apr 17, 2018

lukebatchelor commented Apr 17, 2018

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

alexreardon commented Apr 17, 2018

alexreardon commented Apr 17, 2018

alexreardon commented Apr 17, 2018

TrySound commented Apr 17, 2018

TrySound commented Apr 17, 2018

alexreardon commented Apr 17, 2018