create a build-settings package that all other packages depend on #342
Conversation
kevinbarabash
requested review from
BrianGenisio,
cyejia and
somewhatabstract
as code owners
|
Deploy preview for wonder-blocks ready! Built with commit 9d8bf61 |
Codecov Report
@@ Coverage Diff @@
## master #342 +/- ##
=======================================
Coverage 89.38% 89.38%
=======================================
Files 105 105
Lines 1479 1479
Branches 293 293
=======================================
Hits 1322 1322
Misses 135 135
Partials 22 22Continue to review full report at Codecov.
|
| "version": "0.0.1", | ||
| "main": "index.js", | ||
| "license": "MIT", | ||
| "private": true |
There was a problem hiding this comment.
This isn't going to be published? Do we not having to prefix this with @khanacademy? What happens when we publish the Wonder Blocks packages that depend on this?
There was a problem hiding this comment.
When we go to install one of our wonder-blocks-* packages somewhere, this should be ignored b/c we've marked it as a dev dependency. The @khanacademy prefix isn't necessary but we could add it if we wanted. Since it's not a package we intend on publishing I opted not to include prefix to better differentiate it from those packages we are publishing.
There was a problem hiding this comment.
Hmm - ok! I think a better name would be something like wonder-blocks-build-settings or some such, just to make it clear that this isn't some third-party package.
There was a problem hiding this comment.
I've changed the name of the package in the package.json file. I didn't bother changing the folder name since wonder-blocks/wonder-blocks-build-settings seemed redundant. I also added a README.md in order to try to explain things.
There was a problem hiding this comment.
How do you folks feel about using a naming convention that encapsulates the internal, dev-only nature of this style of package, making that more obvious?
wb-dev-build-settings or just wb-dev-build?
For some reason, it makes me a little uneasy how similar to our prod packages this looks.
There was a problem hiding this comment.
I'll change into wb-dev-build-settings.
build-settings/rollup.config.js
Outdated
| import fs from "fs"; | ||
| import autoExternal from "rollup-plugin-auto-external"; | ||
| import babel from "rollup-plugin-babel"; | ||
|
|
||
| const {presets, plugins} = require("./babel.config.js"); | ||
| const {presets, plugins} = require("../babel.config.js"); |
There was a problem hiding this comment.
Why wasn't the babel config moved too?
There was a problem hiding this comment.
I was worried about it no longer being visible to those things need, but maybe that concern is unfounded since the the things that need are already in this folder.
|
Looks like I need to get the docs building too. |
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable import/no-commonjs */ | |||
There was a problem hiding this comment.
Thank you for adding these!
There was a problem hiding this comment.
I think eventually we'll want want to have two different eslint configs, one for node and one for browser. We should be able to configure eslint to use different configs based on a path regex.
The reason for this is that we want lerna to detect any changes to our build settings and bump the versions of our packages when that happens since the compiled code is different.
It's going to be hard to test this for real b/c I'll need another commit which just changes the
build-settingspackage and none of the other packages. With this PR we've made changes to all packages by either their package.json files whichlernawill detect allowing us to publish new versions of everything.