Check browserslist prop in package.json for environments #1053
Conversation
|
Awesome, thanks for working on this. Could you add a test to cover this change? |
|
I encountered a problem testing this feature/fix. Over here I determine the browserlist-prop to use by checking I also have no idea how to reach the options I pass the bundler inside the tests down to my decision logic. Would you please give me a hint how to continue? I pushed a WIP test case w/ my last commit. |
Codecov Report
@@ Coverage Diff @@
## master #1053 +/- ##
==========================================
- Coverage 88.27% 88.21% -0.07%
==========================================
Files 71 71
Lines 3684 3529 -155
==========================================
- Hits 3252 3113 -139
+ Misses 432 416 -16
Continue to review full report at Codecov.
|
|
I had to refactor things to get the tests running. |
nicolaisueper
force-pushed
the
fix/browserslist-object-environment
branch
2 times, most recently
from
5a56cd6
to
6a20df8
Compare
src/Bundler.js
Outdated
| @@ -72,6 +72,7 @@ class Bundler extends EventEmitter { | |||
| outDir: Path.resolve(options.outDir || 'dist'), | |||
| outFile: options.outFile || '', | |||
| publicURL: publicURL, | |||
| isProduction, | |||
There was a problem hiding this comment.
Could u rename this to production instead of isProduction as we used the key production in the vue branch
src/Parser.js
Outdated
| @@ -73,7 +74,7 @@ class Parser { | |||
| getAsset(filename, pkg, options = {}) { | |||
| let Asset = this.findParser(filename); | |||
| options.parser = this; | |||
| return new Asset(filename, pkg, options); | |||
| return new Asset(filename, pkg, options, this.isProdBuild); | |||
There was a problem hiding this comment.
You don't need to pass this as it's part of the options?
src/Parser.js
Outdated
| @@ -46,6 +46,7 @@ class Parser { | |||
| for (let ext in extensions) { | |||
| this.registerExtension(ext, extensions[ext]); | |||
| } | |||
| this.isProdBuild = options.isProduction; | |||
There was a problem hiding this comment.
You don't need to set this as it's part of the options?
src/assets/JSAsset.js
Outdated
| super(name, pkg, options); | ||
| this.type = 'js'; | ||
| this.globals = new Map(); | ||
| this.isAstDirty = false; | ||
| this.isES6Module = false; | ||
| this.outputCode = null; | ||
| this.cacheData.env = {}; | ||
| this.isProdBuild = isProdBuild; |
There was a problem hiding this comment.
This should normally be part of the this.options
There was a problem hiding this comment.
Oh, you're right
src/transforms/babel.js
Outdated
| @@ -26,8 +26,8 @@ const JSX_PRAGMA = { | |||
| hyperapp: 'h' | |||
| }; | |||
|
|
|||
| async function babelTransform(asset) { | |||
| let config = await getConfig(asset); | |||
| async function babelTransform(asset, isProdBuild) { | |||
There was a problem hiding this comment.
shouldn't be necessary as asset.options.production should return the same as isProdBuild
nicolaisueper
force-pushed
the
fix/browserslist-object-environment
branch
from
6a20df8
to
35304af
Compare
|
I refactored this according to @DeMoorJasper s requests :) |
…com/nicolaisueper/parcel into nicolaisueper-fix/browserslist-object-environment
…isueper-fix/browserslist-object-environment
|
There is no documentation for this feature on Parcel website. |
This will fix #1026.
Please let me know if I should change anything.
Regards
Nico