-
-
Notifications
You must be signed in to change notification settings - Fork 451
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
add babel-core and preset-2015 to dev dependencies #273
Conversation
Nice thanks, can you then pls also remove them from the |
@@ -16,6 +16,8 @@ | |||
"webpack": "1 || ^2.1.0-beta" | |||
}, | |||
"devDependencies": { | |||
"babel-core": "^6.0.0", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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
4c23514
to
3a1c832
Compare
done and rebased against latest head |
Current coverage is 96.72% (diff: 100%)@@ 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
|
Thanks. |
* 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
currently it is impossible to run tests after forking the
babel-loader
without installingbabel-core
andbabel-preset-2015
by hand. Adding them to the dev-dependencies will fix this. (I saw the dependencies as they are installed in thebefore_script
according to the.travis.yml
)