Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add babel-core and preset-2015 to dev dependencies #273

Merged

Conversation

timse
Copy link
Contributor

@timse timse commented Aug 4, 2016

currently it is impossible to run tests after forking the babel-loader without installing babel-core and babel-preset-2015 by hand. Adding them to the dev-dependencies will fix this. (I saw the dependencies as they are installed in the before_script according to the .travis.yml)

@danez
Copy link
Member

danez commented Aug 6, 2016

Nice thanks, can you then pls also remove them from the .travis.yml. That would be nice

@@ -16,6 +16,8 @@
"webpack": "1 || ^2.1.0-beta"
},
"devDependencies": {
"babel-core": "^6.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Imho babel-core needs to be a dependency and not devDependency as it is used at runtime. The Preset is probably only used in the tests.

Copy link
Contributor

@joshwiens joshwiens Aug 7, 2016

Choose a reason for hiding this comment

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

Which is why it is a peerDependency. At what point would someone try and use this plugin without already having installed babel-core or had it come along with something else babel related?

We really shouldn't be packaging babel-core in the webpack loader imo.

If it's explicitly added - you're good
If it's a dependency of X - you're good
If for some strange reason neither of the above are true, you get a peerDependency warning and need to npm i

This also allows someone to be explicit about the version of babel-core they are using which when I look at this as a consumer, is something I would prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I was reading you want to get rid of the peer dependency. but yeah if the peer dependency stays then we don't have to add it to dependencies

@timse
Copy link
Contributor Author

timse commented Aug 8, 2016

sure, will do :)

currently it is impossible to run tests after forking the `babel-loader` without installing "babel-core" and "babel-preset-2015"
adding them to the dev-dependencies will fix this. (I saw the dependencies as they are installed in the "before_script" according to the .travis.yml
this is now done on npm install as they are devDependencies, so installing them again is redundant
@timse timse force-pushed the add-babel-core-and-presets-as-dev-dependencies branch from 4c23514 to 3a1c832 Compare August 8, 2016 15:50
@timse
Copy link
Contributor Author

timse commented Aug 8, 2016

done and rebased against latest head

@codecov-io
Copy link

codecov-io commented Aug 8, 2016

Current coverage is 96.72% (diff: 100%)

Merging #273 into master will not change coverage

@@             master       #273   diff @@
==========================================
  Files             5          5          
  Lines           122        122          
  Methods          19         19          
  Messages          0          0          
  Branches         26         26          
==========================================
  Hits            118        118          
  Misses            4          4          
  Partials          0          0          

Powered by Codecov. Last update 1753f27...3a1c832

@danez
Copy link
Member

danez commented Aug 8, 2016

Thanks.

@danez danez merged commit 2b139f3 into babel:master Aug 8, 2016
Ognian pushed a commit to Ognian/babel-loader that referenced this pull request Feb 27, 2017
* add babel-core and preset-2015 to dev dependencies

currently it is impossible to run tests after forking the `babel-loader` without installing "babel-core" and "babel-preset-2015"
adding them to the dev-dependencies will fix this. (I saw the dependencies as they are installed in the "before_script" according to the .travis.yml

* no longer install `babel-core` and `babel-preset-es2015` in travis

this is now done on npm install as they are devDependencies, so installing them again is redundant
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.

None yet

4 participants