Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Add watch script for dev #234

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Add watch script for dev #234

merged 1 commit into from
Dec 5, 2016

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 24, 2016

Q A
Bug fix? no
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? no
Tests added/pass? no
Fixed tickets comma-separated list of tickets fixed by the PR, if any
License MIT

The watch script was removed in this PR when we started bundling Babylon with rollup (which is awesome!). This PR sets up a watch script for us (the big difference is that with Rollup the es2015 preset's modules option must be false, while we need to enable transpiling to CommonJS modules when Babel is run).

I tried Rollup's watcher, but it is currently WIP, and would crash and was unable to recover from saving files with syntax errors when I tried it.

This is more of a stop gap measure until the Rollup watcher is ready for primetime.

Thoughts on this? I personally was using the watcher and have continued to set up something like this up locally when I'm working on Babylon, so figured it might be nice to have a watch script again. Totally understand we probably want to use one of the two solutions above, but I figured having a watcher is better than not :)

@kaicataldo kaicataldo force-pushed the add-watcher branch 2 times, most recently from 6e46c3c to 73f2fda Compare November 24, 2016 06:17
@kaicataldo kaicataldo changed the title Add watcher for dev Add watch script for dev Nov 24, 2016
@danez
Copy link
Member

danez commented Nov 25, 2016

It would be nice in general that the tests maybe not use the rollup bundle, so that we can have proper unit tests, right now the only thing that can be tested is the public API.
And then use rollup only for publishing. Although it is probably also nice to know that the rollup bundle works as expected.

@danez
Copy link
Member

danez commented Nov 26, 2016

I changed it so that the tests run without rollup.
@DrewML Do you think this is okay?

"stage-0"
]
},
"development": {
Copy link
Member Author

@kaicataldo kaicataldo Nov 26, 2016

Choose a reason for hiding this comment

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

I wonder if we should just combine development and test, since they're the same? Maybe just production and development?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see, it's for the plugins. Never mind!

@DrewML
Copy link
Member

DrewML commented Nov 26, 2016

I changed it so that the tests run without rollup.
@DrewML Do you think this is okay?

I think the only part that would be concerning is not running any tests against the bundle in CI. It's possible rollup could introduce a bug that we would never catch until running the Babel test suite against a release of Babylon.

Edit: Thinking about it more, we run the Babel test suite in CI against each Babylon PR, right?

@kaicataldo
Copy link
Member Author

Thinking about it more, we run the Babel test suite in CI against each Babylon PR, right?

Yeah! Seems like we could run that CI test against the bundle?

It would be nice in general that the tests maybe not use the rollup bundle, so that we can have proper unit tests, right now the only thing that can be tested is the public API.

Could it make sense to split this out into unit and integration/behavioral tests and not bundle for the first and then bundle for the latter?

@kaicataldo
Copy link
Member Author

kaicataldo commented Dec 1, 2016

Maybe it would make more sense to separate these two tasks out? I don't think these two things have to be tied together. Seems like we could just do the watcher script in this one and we then have the env's to take it further for testing, etc. in a future PR? My intention with this was just to add a convenience for us devs :)

@hzoo
Copy link
Member

hzoo commented Dec 1, 2016

I think @kaicataldo just wanted to be able to watch in dev as this PR. If it's simple enough we can just land that and then work on the other stuff. Also keep in mind we probably want to just move it back into the monorepo as well 😝

@danez
Copy link
Member

danez commented Dec 3, 2016

Yeah true, feel free to reset the branch to your commit and force push.

@kaicataldo
Copy link
Member Author

Updated to just add a watch script

@kaicataldo kaicataldo removed the PR: WIP label Dec 5, 2016
@@ -10,6 +10,14 @@
"transform-flow-strip-types"
],
"env": {
"watch": {
"presets": [
["es2015", {
Copy link
Member

Choose a reason for hiding this comment

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

might be able to just do plugins commonjs here due to the inheriting of the top level presets/plugins but this works fine

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh yeah that could be better 👍

@hzoo hzoo merged commit 4072dfd into babel:master Dec 5, 2016
@hzoo
Copy link
Member

hzoo commented Dec 5, 2016

👍

@kaicataldo kaicataldo deleted the add-watcher branch December 5, 2016 21:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants