Change build system and update Jest #747

emmatown · 2018-06-30T03:19:08Z

What:
This changes the build system from some npm scripts to some node scripts that run rollup on every package. This is extracted from https://github.com/emotion-js/next

This also moves the site out of the packages folder into the root directory because I wanted to make the packages folder only contain packages that are published to npm. The benchmarks package was also moved to the new scripts folder for the same reason.

Why:

Faster builds since a new process doesn't need to be started for each build
Everything runs through the same build system

How:

Checklist:

Documentation N/A
Tests
Code complete

codecov · 2018-06-30T03:25:25Z

Codecov Report

Merging #747 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/babel-plugin-emotion/src/macro-styled.js	`100% <ø> (ø)`	⬆️
packages/create-emotion-server/src/index.js	`100% <ø> (ø)`	⬆️
packages/babel-plugin-emotion/src/macro.js	`100% <ø> (ø)`	⬆️
.../build/add-basic-constructor-to-react-component.js	`100% <100%> (ø)`
packages/babel-plugin-emotion/src/index.js	`96.88% <100%> (+0.01%)`	⬆️

Andarist · 2018-06-30T04:20:08Z

packages/babel-plugin-emotion/lib/macro-styled.js

@@ -0,0 +1 @@
+module.exports = require('babel-plugin-emotion').macros.styled


whats the reason behind those 2 "proxy" files?

In case people have custom instances of emotion and they're using the macros with them since this they were previously imported via babel-plugin-emotion/lib/macro and babel-plugin-emotion/lib/macro-styled

gotcha, maybe add a comment that this can be removed with next major?

Andarist · 2018-06-30T04:22:20Z

packages/create-emotion-styled/package.json

-    "prop-types": "^15.6.1",
-    "rimraf": "^2.6.1",
-    "rollup": "^0.60.1"
+    "prop-types": "^15.6.1"


seems weird to have prop-types in devDeps, are they needed here?

It was already there. It's probably not strictly necessary but it was probably added there because it's a peerDependency and it's needed in dev. If you think it should be removed them I'll remove it but IMO, it doesn't really matter.

peer dep thing makes sense 👍

Andarist · 2018-06-30T04:35:49Z

scripts/build/add-basic-constructor-to-react-component.js

+    name: 'ast-transform', // not required
+    visitor: {
+      Class(path) {
+        if (


could be made more readable with buildMatchMemberExpression & I would extract hasConstructor too

const isReactComponent = t.buildMatchMemberExpression('React.Component') // ... if (isReactComponent(path.node.superClass) && hasConstructor(path.node.body.body)) { // ... }

Andarist · 2018-06-30T04:39:37Z

scripts/build/add-basic-constructor-to-react-component.js

+            )
+          })
+        ) {
+          path.node.body.body.unshift(


lately i've become fan of template, IMHO it increases readability a lot - although not sure now if it will be possible to create a class method with it - we could always create dummy class with constructor only and just unpack constructor node though

Andarist · 2018-06-30T04:41:58Z

scripts/build/add-basic-constructor-to-react-component.js

@@ -0,0 +1,40 @@
+// this is to avoid the spread argument helper for react component constructors


could u explain what exactly is avoided with this and why this is a problem?

https://runkit.com/mitchellhamilton/5b370eeaf5b34c0012874d92
Without this, the constructor's arguments will be iterated over and put in an array unnecessarily.

nice! this would be useful as part of preset-react, gonna propose it later to rest of the babel team

BTW, this implementation isn't perfect since it should technically pass the second argument to super as well (and maybe the third but I don't know if that's strictly necessary). The reason I didn't do that here is that I know I won't access context in those cases.

https://github.com/facebook/react/blob/64e1921aab4f7ad7e2f345f392033ffe724e52e9/packages/react/src/ReactBaseClasses.js#L21-L29

no idea how updater arg might be used 🤔 we could do super(props, context, updater) by default + an option to optimize it to just super(props)

Not sure if context here is only for legacy purposes, or is the new one also passed here - anyway doesn't matter as long as this arg is here

Andarist · 2018-06-30T04:45:39Z

Quite a lengthy PR 😉 have to go to sleep now, but gonna finish reviewing in few hours

Andarist · 2018-06-30T08:26:03Z

scripts/build/fix-dce-for-classes-with-statics.js

+}
+
+function isPrimitive(val) {
+  return val == null || /^[sbn]/.test(typeof val)


i understand this regexp, but it looks slightly confusing at first - i'm unsure if u i.e. meant to catch symbol with it or not? would prefer inlining all possibilities here for clarity, especially that this is not a bytes-sensitive code

Andarist · 2018-06-30T08:28:59Z

scripts/build/package.json

+    "del": "^3.0.0",
+    "rollup": "^0.62.0",
+    "rollup-plugin-alias": "^1.4.0",
+    "rollup-plugin-babel": "^4.0.0-beta.5",


it's safer to pin beta packages to exact versions, also - this could get upgraded, there is beta.7 already

Andarist · 2018-06-30T08:29:17Z

scripts/build/package.json

@@ -0,0 +1,16 @@
+{
+  "name": "build",
+  "version": "0.5.7",


does it make sense to version this?

I think some tools(I think it's lerna and yarn but I'm not totally sure) complain when there isn't a version so since it doesn't really hurt anything, we might as well leave it.

Andarist · 2018-06-30T08:33:47Z

scripts/build/inline-isBrowser.js

+
+        if (!path.node.specifiers.length) {
+          path.remove()
+        }


as this is used as filter predicate would be good to return explicit boolean here

This part isn't in the filter predicate.

oh, right, my bad 👍

Andarist · 2018-06-30T08:35:39Z

scripts/build/rollup.config.js

+          ['@babel/proposal-class-properties', { loose: true }],
+          require('./fix-dce-for-classes-with-statics'),
+          isBrowser && require('./inline-isBrowser'),
+          isBrowser &&


isnt this duplicating what inline-isBrowser.js do (except filtering imported specifiers)? why there are 2 similar plugins?

inline-isBrowser.js is replacing the usages of @emotion/utils in packages whereas this plugin is changing the declarations inside of @emotion/utils

could we maybe add a filename guard to this second plugin so it wont run accidentally on other packages? I think there are some nice new options in babel@7 - like overrides etc, maybe they could be leveraged to add this plugin only for certain filepaths/directories, this would make the intent more explicit

Andarist · 2018-06-30T08:37:28Z

scripts/build/rollup.config.js

+                }
+              }
+            }),
+          '@babel/plugin-proposal-object-rest-spread'


might be a good moment to update babel@7 deps too and use loose for this plugin

Andarist · 2018-06-30T08:39:38Z

scripts/build/utils.js

@@ -0,0 +1,98 @@
+const path = require('path')
+const del = require('del')


seems like both rimraf and del are used across the codebase - not a big deal, but could be unified

I think the usages of rimraf were only in npm scripts and I removed those ones so I don't think rimraf is being used anymore.

Andarist · 2018-06-30T08:52:45Z

scripts/build/change-packages.js

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


is this file used? cant find the usage in this PR, I also don't quite get why this is needed - especially when you have removed those devDeps etc in this PR

or is it a helper script which u have used to modify packages in this PR + it can be used to modify them in the future? if so it would be great to add a comment about this

It's a helper script that can be used to modify the packages in the future, I'll add a comment.

Andarist · 2018-06-30T10:42:17Z

The whole idea & setup looks way better than previous one 👍

I feel though that monorepo setup should be something "on top" of regular git repository - it's job is to ease dependency management & local development. IMHO even if it's kinda artificial problem most of the time I always strive for separating packages, their tooling etc - what I mean by that is that having a monorepo script i.e. to build stuff is a good thing for development but even having such it would be good to embed build tooling + devDeps etc in each package so it can be easily extracted from the monorepo and moved away. Proposed build setup could be packaged and handle both monorepo and single package use cases. I understand it's not really important right now so I propose to screw that for now, but create an issue about this once this PR gets merged.

Andarist · 2018-06-30T10:47:46Z

scripts/build/utils.js

+    }
+    if (ret.pkg['umd:main']) {
+      ret.configs.push({
+        config: makeRollupConfig(ret, true, true),


would prefer changing signature to:

makeRollupConfig(data, { isBrowser = false, isUmd = false } = {}) { // ... }

right now looking at this call site it's hard to tell what those booleans do

not yet sure about this but - I can't find a package using "browser" field, so not sure how exactly we need to handle it, but a good alternative to transforming packages with babel and replacing stuff with browser values it might be clearer to introduce BROWSER env variable, produce code conditionally with process.env ternaries in the actual code and prepare 2 builds with appropriate rollup-plugin-replace setup

The reason I didn't do that is I want to ship flow types from the source in v10 and using a global would require adding a declare var to every usage of it so that people don't get flow errors from it.

…s because the filename that babel passes now includes the root directory, this won't change user hashes)

Andarist · 2018-07-01T09:10:03Z

Continuing #747 (comment)

Should be possible to prepare multiple "source" directories - with only process.env replaced in them and build final bundles from them + ofc redirecting types to appropriate directories. It would be quite unconventional setup and slightly more complicated, but IMHO would be clearer in the actual source what are the possible code shapes, right now the babel plugin idea is hidden and can be broken more easily.

emmatown · 2018-07-01T09:34:13Z

I was thinking of using plain globals but using process.env would probably just work since flow knows about process.env.

The other problem though is that the second variable shouldSerializeToReactTree, also depends on process.env.NODE_ENV when it's not in the browser build so either the check for that would have to be inlined or imported from @emotion/utils which is just an extra thing to deal with.

TBH, I think that even though it's not perfect, I'd rather just leave it right now since it's not getting used on the current packages here yet and it works.

TrySound · 2018-07-01T09:50:53Z

Would be good to change es suffix to esm which is more descriptive like esm loader. The latest version of rollup also has esm alias for format which will replace es in the future. material-ui uses es to ship es6 (not only modules) syntax.

This reverts commit 07aaace.

emmatown · 2018-07-02T01:31:33Z

Thanks for your review @Andarist!!

Andarist · 2018-07-02T17:53:15Z

Not a problem, it was a cool PR to review! 😃

emmatown added 16 commits May 5, 2018 21:28

Start changing build stuff

e9315c6

Fix stuff

ef6dd5c

Merge branch 'master' into change-build

2ee93eb

Move site and benchmarks

1a46819

Merge branch 'master' into change-build

46b2750

Add a thing for flow

f2f153f

Change some stuff

df609e9

Add UMD builds and update build script from next

f9c975d

Fix stuff

c74a39a

Clean up stuff and update jest

21d2b01

Change a thing

95840eb

Merge branch 'master' into change-build

6e1256c

Change a thing

c5cabe5

Fix site

1b8e73b

probably fix ci

541b1cd

Fix linting

edf6af3

emmatown added the Tag: Internal label Jun 30, 2018

emmatown requested review from tkh44 and Andarist June 30, 2018 03:19

emmatown added 2 commits June 30, 2018 13:23

Update CONTRIBUTING.md

eea5e74

Change netlify.toml

4decc39

Andarist reviewed Jun 30, 2018

View reviewed changes

Fix path

86c95dc

Andarist reviewed Jun 30, 2018

View reviewed changes

Fix more paths

b70004d

Andarist reviewed Jun 30, 2018

View reviewed changes

emmatown added 2 commits June 30, 2018 19:23

Address some feedback and stuff

43eb9e3

Support PureComponent in basic react constructor babel plugin

d31ded1

Andarist reviewed Jun 30, 2018

View reviewed changes

emmatown added 6 commits July 1, 2018 12:27

Add flow types to build script

6f89776

Fix a thing

44e35e9

Update babel (lots of tests are failing)

29887b9

Fix mock fs in babel plugin tests

7ae8d49

Update snapshots (the reason the component selector hashes changed wa…

200495c

…s because the filename that babel passes now includes the root directory, this won't change user hashes)

Add to coverage ignore

3a0fe42

Fix site and switch benchmarks to parcel

07aaace

emmatown added 4 commits July 1, 2018 20:36

Revert "Fix site and switch benchmarks to parcel"

6d552a4

This reverts commit 07aaace.

Add browserslist to resolutions

6d9f93b

Change es suffix to esm

cb4e562

Fix site

ec46278

emmatown merged commit aa8c4e4 into master Jul 2, 2018

emmatown deleted the change-build branch July 2, 2018 01:31

		@@ -0,0 +1 @@
		module.exports = require('babel-plugin-emotion').macros.styled

		@@ -0,0 +1,40 @@
		// this is to avoid the spread argument helper for react component constructors

		@@ -0,0 +1,98 @@
		const path = require('path')
		const del = require('del')

Change build system and update Jest #747

Change build system and update Jest #747

Conversation

emmatown commented Jun 30, 2018 • edited Loading

codecov bot commented Jun 30, 2018 • edited Loading

Codecov Report

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

emmatown Jun 30, 2018 • edited by Andarist Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Jun 30, 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

emmatown Jun 30, 2018 • edited Loading

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

Andarist commented Jun 30, 2018

Andarist Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Jul 1, 2018

emmatown commented Jul 1, 2018

TrySound commented Jul 1, 2018

emmatown commented Jul 2, 2018

Andarist commented Jul 2, 2018

emmatown commented Jun 30, 2018 •

edited

Loading

codecov bot commented Jun 30, 2018 •

edited

Loading

emmatown Jun 30, 2018 •

edited by Andarist

Loading

emmatown Jun 30, 2018 •

edited

Loading

Andarist Jun 30, 2018 •

edited

Loading