Change build system and update Jest #747
Conversation
Codecov Report
|
| @@ -0,0 +1 @@ | |||
| module.exports = require('babel-plugin-emotion').macros.styled | |||
There was a problem hiding this comment.
whats the reason behind those 2 "proxy" files?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
gotcha, maybe add a comment that this can be removed with next major?
| "prop-types": "^15.6.1", | ||
| "rimraf": "^2.6.1", | ||
| "rollup": "^0.60.1" | ||
| "prop-types": "^15.6.1" |
There was a problem hiding this comment.
seems weird to have prop-types in devDeps, are they needed here?
There was a problem hiding this comment.
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.
| name: 'ast-transform', // not required | ||
| visitor: { | ||
| Class(path) { | ||
| if ( |
There was a problem hiding this comment.
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)) {
// ...
}
| ) | ||
| }) | ||
| ) { | ||
| path.node.body.body.unshift( |
There was a problem hiding this comment.
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
| @@ -0,0 +1,40 @@ | |||
| // this is to avoid the spread argument helper for react component constructors | |||
There was a problem hiding this comment.
could u explain what exactly is avoided with this and why this is a problem?
There was a problem hiding this comment.
https://runkit.com/mitchellhamilton/5b370eeaf5b34c0012874d92
Without this, the constructor's arguments will be iterated over and put in an array unnecessarily.
There was a problem hiding this comment.
nice! this would be useful as part of preset-react, gonna propose it later to rest of the babel team
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Quite a lengthy PR 😉 have to go to sleep now, but gonna finish reviewing in few hours |
| } | ||
|
|
||
| function isPrimitive(val) { | ||
| return val == null || /^[sbn]/.test(typeof val) |
There was a problem hiding this comment.
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
scripts/build/package.json
Outdated
| "del": "^3.0.0", | ||
| "rollup": "^0.62.0", | ||
| "rollup-plugin-alias": "^1.4.0", | ||
| "rollup-plugin-babel": "^4.0.0-beta.5", |
There was a problem hiding this comment.
it's safer to pin beta packages to exact versions, also - this could get upgraded, there is beta.7 already
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "name": "build", | |||
| "version": "0.5.7", | |||
There was a problem hiding this comment.
does it make sense to version this?
There was a problem hiding this comment.
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.
|
|
||
| if (!path.node.specifiers.length) { | ||
| path.remove() | ||
| } |
There was a problem hiding this comment.
as this is used as filter predicate would be good to return explicit boolean here
There was a problem hiding this comment.
This part isn't in the filter predicate.
| ['@babel/proposal-class-properties', { loose: true }], | ||
| require('./fix-dce-for-classes-with-statics'), | ||
| isBrowser && require('./inline-isBrowser'), | ||
| isBrowser && |
There was a problem hiding this comment.
isnt this duplicating what inline-isBrowser.js do (except filtering imported specifiers)? why there are 2 similar plugins?
There was a problem hiding this comment.
inline-isBrowser.js is replacing the usages of @emotion/utils in packages whereas this plugin is changing the declarations inside of @emotion/utils
There was a problem hiding this comment.
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
scripts/build/rollup.config.js
Outdated
| } | ||
| } | ||
| }), | ||
| '@babel/plugin-proposal-object-rest-spread' |
There was a problem hiding this comment.
might be a good moment to update babel@7 deps too and use loose for this plugin
| @@ -0,0 +1,98 @@ | |||
| const path = require('path') | |||
| const del = require('del') | |||
There was a problem hiding this comment.
seems like both rimraf and del are used across the codebase - not a big deal, but could be unified
There was a problem hiding this comment.
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.
| @@ -0,0 +1,45 @@ | |||
| // @flow | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
It's a helper script that can be used to modify the packages in the future, I'll add a comment.
|
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. |
scripts/build/utils.js
Outdated
| } | ||
| if (ret.pkg['umd:main']) { | ||
| ret.configs.push({ | ||
| config: makeRollupConfig(ret, true, true), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
|
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. |
|
I was thinking of using plain globals but using The other problem though is that the second variable 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. |
|
Would be good to change |
|
Thanks for your review @Andarist!! |
|
Not a problem, it was a cool PR to review! 😃 |
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:
How:
Checklist: