This repository has been archived by the owner. It is now read-only.

Add forceAllTransforms option and deprecate Uglify target #264

Merged
merged 1 commit into from Apr 26, 2017

Conversation

Projects
None yet
5 participants
@existentialism
Member

existentialism commented Apr 10, 2017

Fixes #234.

Added as a deprecation, but can straight remove if we want?

Also, still unsure about the name (useTargetSyntax?)?

@existentialism existentialism added this to the 2.0 milestone Apr 10, 2017

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Apr 10, 2017

Member

👍

Also not sure about useSyntax naming. useTargetSyntax maybe can be confusing (we need to specifify targetSyntax or targets are syntax targets) 🙃.

Maybe focus on the code transformation? useTransformations/useTransform?

Or combine all the thoughts and useuseSyntaxTransform?

Member

yavorsky commented Apr 10, 2017

👍

Also not sure about useSyntax naming. useTargetSyntax maybe can be confusing (we need to specifify targetSyntax or targets are syntax targets) 🙃.

Maybe focus on the code transformation? useTransformations/useTransform?

Or combine all the thoughts and useuseSyntaxTransform?

@yavorsky yavorsky referenced this pull request Apr 11, 2017

Merged

Add flow #269

@existentialism

This comment has been minimized.

Show comment
Hide comment
@existentialism

existentialism Apr 14, 2017

Member

@yavorsky thoughts on forceAllTransforms?

With a .babelrc.js you would have:

module.exports = {
  presets: [
    ["env", {
      targets: {
        chrome: 59,
        edge: 13,
        firefox: 50,
      },
      // for uglifyjs...
      forceAllTransforms: process.env === "production"
    }],
  ],
};
Member

existentialism commented Apr 14, 2017

@yavorsky thoughts on forceAllTransforms?

With a .babelrc.js you would have:

module.exports = {
  presets: [
    ["env", {
      targets: {
        chrome: 59,
        edge: 13,
        firefox: 50,
      },
      // for uglifyjs...
      forceAllTransforms: process.env === "production"
    }],
  ],
};
@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Apr 20, 2017

Member

forceAllTransforms looks better. 👍
But on the other hand, useBuiltIns: "entry" make some transforms also. 🤔

Member

yavorsky commented Apr 20, 2017

forceAllTransforms looks better. 👍
But on the other hand, useBuiltIns: "entry" make some transforms also. 🤔

@existentialism

This comment has been minimized.

Show comment
Hide comment
@existentialism

existentialism Apr 20, 2017

Member

@yavorsky @hzoo updated the name to forceAllTransforms... can land and rename it later?

Member

existentialism commented Apr 20, 2017

@yavorsky @hzoo updated the name to forceAllTransforms... can land and rename it later?

@existentialism existentialism changed the title from Add useSyntax option and deprecate Uglify target to Add forceAllTransforms option and deprecate Uglify target Apr 21, 2017

@existentialism existentialism requested a review from hzoo Apr 21, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Apr 21, 2017

Codecov Report

Merging #264 into 2.0 will decrease coverage by 0.43%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##              2.0     #264      +/-   ##
==========================================
- Coverage   96.87%   96.43%   -0.44%     
==========================================
  Files           9        9              
  Lines         384      393       +9     
  Branches      124      126       +2     
==========================================
+ Hits          372      379       +7     
- Misses          3        4       +1     
- Partials        9       10       +1
Impacted Files Coverage Δ
src/normalize-options.js 100% <100%> (ø) ⬆️
src/index.js 97.67% <100%> (-2.33%) ⬇️
src/targets-parser.js 96.29% <81.81%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2b3401...a18d636. Read the comment docs.

codecov-io commented Apr 21, 2017

Codecov Report

Merging #264 into 2.0 will decrease coverage by 0.43%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##              2.0     #264      +/-   ##
==========================================
- Coverage   96.87%   96.43%   -0.44%     
==========================================
  Files           9        9              
  Lines         384      393       +9     
  Branches      124      126       +2     
==========================================
+ Hits          372      379       +7     
- Misses          3        4       +1     
- Partials        9       10       +1
Impacted Files Coverage Δ
src/normalize-options.js 100% <100%> (ø) ⬆️
src/index.js 97.67% <100%> (-2.33%) ⬇️
src/targets-parser.js 96.29% <81.81%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2b3401...a18d636. Read the comment docs.

@hzoo

hzoo approved these changes Apr 26, 2017

@existentialism existentialism merged commit 4ddd615 into 2.0 Apr 26, 2017

2 of 4 checks passed

codecov/patch 92.3% of diff hit (target 96.87%)
Details
codecov/project 96.43% (-0.44%) compared to b2b3401
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@existentialism existentialism deleted the use-syntax-option branch Apr 26, 2017

existentialism added a commit that referenced this pull request Apr 26, 2017

@nodkz

This comment has been minimized.

Show comment
Hide comment
@nodkz

nodkz Aug 10, 2017

@existentialism @yavorsky can you add a prefix to your deprecation messages?

I update dependencies, start a new production build and got such message:

screen shot 2017-08-10 at 17 43 55

Firstly I think that problem in new Webpack 3.5, but its and UglifyPlugin source code does not contain such massage. And look in my .babelrc.js and found targets.uglify: true for production build there.

in your code you use console.log, maybe you add wrapper over your messages deprecate, info, error etc?
Thanks.

nodkz commented Aug 10, 2017

@existentialism @yavorsky can you add a prefix to your deprecation messages?

I update dependencies, start a new production build and got such message:

screen shot 2017-08-10 at 17 43 55

Firstly I think that problem in new Webpack 3.5, but its and UglifyPlugin source code does not contain such massage. And look in my .babelrc.js and found targets.uglify: true for production build there.

in your code you use console.log, maybe you add wrapper over your messages deprecate, info, error etc?
Thanks.

@existentialism

This comment has been minimized.

Show comment
Hide comment
Member

existentialism commented Aug 10, 2017

@nodkz 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.