Skip to content

Conversation

@micaww
Copy link
Contributor

@micaww micaww commented Oct 4, 2019

Reverts the previous merge that removes destructuring usages in favor of a build step similar to clay-kiln, in order to ensure ES5 output compatible with all browsers.

Adds the following npm scripts:

  • npm run prepublishOnly for a production build before publishing to npm
  • npm run build or npm run watch for a development build

Only the output bundle will be published to npm, rather than the source/test suite.

@coveralls
Copy link

coveralls commented Oct 4, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 367a79b on Entercom:add-build-step into 99791ff on clay:master.

.babelrc Outdated
"presets": [
["@babel/preset-env", {
"targets": {
"browsers": ["last 2 versions"]
Copy link

@olsonpm olsonpm Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want last 2 versions ? I'd assume you'd want browserslist's "defaults"

https://github.com/browserslist/browserslist#best-practices

Edit: also the babel docs have a note by targets.browsers

https://babeljs.io/docs/en/babel-preset-env#targetsbrowsers

Note: this will be removed in later version in favor of just setting "targets" to a query directly.

so I think instead you'd just want "targets": <browserslist query>

https://babeljs.io/docs/en/babel-preset-env#targets

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last 2 versions only because that's what clay-kiln uses. defaults might be better anyways since it's more broad.

Copy link

@olsonpm olsonpm Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah - I didn't realize that was in sync with clay-kiln. Using whatever clay-kiln has is probably best then to keep things consistent :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or defaults haha - maybe the clay team will have an opinion

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that supports the version clay-kiln uses is fine. defaults sounds like a good choice.

@jpope19 jpope19 merged commit 3840dbd into clay:master Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants