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

fix: pin strip-ansi to 3.x for ES5 compat #1273

Merged
merged 1 commit into from Jan 19, 2018
Merged

Conversation

@yyx990803
Copy link
Contributor

@yyx990803 yyx990803 commented Jan 14, 2018

  • This is a bugfix

Follow up for #1270 (comment)

strip-ansi which is required by the clients, uses ES6 syntax. Pinning it to 3.x keeps ES5 compat. (The only difference between 3.x and 4.0 is the syntax change)

@codecov
Copy link

@codecov codecov bot commented Jan 14, 2018

Codecov Report

Merging #1273 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1273   +/-   ##
=======================================
  Coverage   75.72%   75.72%           
=======================================
  Files           5        5           
  Lines         482      482           
  Branches      156      156           
=======================================
  Hits          365      365           
  Misses        117      117

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 8c1ed7a...b78e249. Read the comment docs.

@graingert
Copy link
Contributor

@graingert graingert commented Jan 16, 2018

@sokra if you don't want to play whack-a-mole with deps as the npm ecosystem moves to releasing greenfield JS you should have a look at #1279

@julianxhokaxhiu
Copy link

@julianxhokaxhiu julianxhokaxhiu commented Jan 18, 2018

Are you planning on merging and releasing this ASAP? I am also impacted by the same identical issue at the moment.

@graingert
Copy link
Contributor

@graingert graingert commented Jan 18, 2018

@julianxhokaxhiu FYI, i've released drop-in-replacement @procensus/webpack-dev-server with my fix in #1279

@julianxhokaxhiu
Copy link

@julianxhokaxhiu julianxhokaxhiu commented Jan 18, 2018

Thanks, I'll use that temporary until this gets merged :)

@graingert
Copy link
Contributor

@graingert graingert commented Jan 18, 2018

this gets merged :)

Hopefully it won't! ;) publishing `ES${new @Date().getFullYear()}` to npm is the future. See #1279

@julianxhokaxhiu
Copy link

@julianxhokaxhiu julianxhokaxhiu commented Jan 18, 2018

I tested your drop-in replacement and I still get the same error:

Syntax error on L2114:
module.exports = input => typeof input === 'string' ? input.replace(ansiRegex(), '') : input;

it seems that the ansi-module is not transpiled from arrow functions.

//EDIT: See also vercel/next.js#2747

The solution there was to downgrade strip-ansi: https://github.com/zeit/next.js/pull/2860/commits

So this PR is really needed.

@graingert
Copy link
Contributor

@graingert graingert commented Jan 18, 2018

@yyx990803
Copy link
Contributor Author

@yyx990803 yyx990803 commented Jan 18, 2018

@graingert FYI #1279 does not fix CLI --inline usage because injected entries will be processed with user config.

julianxhokaxhiu added a commit to julianxhokaxhiu/polysticky.js that referenced this pull request Jan 19, 2018
Enabled serving on IE11 meanwhile webpack/webpack-dev-server#1273 gets merged
@TheLarkInn TheLarkInn self-requested a review Jan 19, 2018
@TheLarkInn TheLarkInn merged commit 3aa15aa into webpack:master Jan 19, 2018
5 checks passed
5 checks passed
Codacy/PR Quality Review Good work! A positive pull request.
Details
codecov/patch Coverage not affected when comparing 8c1ed7a...b78e249
Details
codecov/project 75.72% remains the same compared to 8c1ed7a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@TheLarkInn
Copy link
Member

@TheLarkInn TheLarkInn commented Jan 19, 2018

@yyx990803 I think this looks fine.

@TheLarkInn
Copy link
Member

@TheLarkInn TheLarkInn commented Jan 19, 2018

Would someone like to submit an issue so that we can upgrade in the future but not break IE? Pinning versions is a sort of a bandaid IMO.

@TheLarkInn
Copy link
Member

@TheLarkInn TheLarkInn commented Jan 19, 2018

Nevermind, I got it tracked in issues #1286

@TheLarkInn
Copy link
Member

@TheLarkInn TheLarkInn commented Jan 19, 2018

I've got limited time today to publish this, however I will see if I can get @SpaceK33z or @sokra to run a release for me on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants