Skip to content

refactor(ES6): upgrade APIPlugin to ES6#3675

Merged
TheLarkInn merged 4 commits intowebpack:masterfrom
willmendesneto:refactor-api-plugin
Jan 2, 2017
Merged

refactor(ES6): upgrade APIPlugin to ES6#3675
TheLarkInn merged 4 commits intowebpack:masterfrom
willmendesneto:refactor-api-plugin

Conversation

@willmendesneto
Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

ES5 => ES6 refactor

Did you add tests for your changes?

refactor, all existing tests pass

If relevant, link to documentation update:

N/A

Summary

Helping in part of the ES6 refactor

Does this PR introduce a breaking change?

Nope

@jsf-clabot
Copy link
Copy Markdown

jsf-clabot commented Jan 2, 2017

CLA assistant check
All committers have signed the CLA.

Comment thread lib/APIPlugin.js Outdated
parser.plugin("expression " + key, expr => {
const dep = new ConstDependency(REPLACEMENTS[key], expr.range);
dep.loc = expr.loc;
this.state.current.addDependency(dep);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be parser here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks for the feedback @sokra !

@willmendesneto
Copy link
Copy Markdown
Contributor Author

@sokra Can you take a look again, please?

Copy link
Copy Markdown
Contributor

@chicoxyzzy chicoxyzzy left a comment

Choose a reason for hiding this comment

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

You also can rewrite all string concatenations as template strings

Copy link
Copy Markdown
Contributor

@chicoxyzzy chicoxyzzy left a comment

Choose a reason for hiding this comment

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

Should be squashed IMO otherwise LGTM 👍

@willmendesneto
Copy link
Copy Markdown
Contributor Author

Should be squashed IMO otherwise LGTM

Yes, for sure (good catch @chicoxyzzy ). We shouldn't add broken commits inside master branch. I don't know for now if we are not using squash as standard, but if not it's a good idea add it.

@TheLarkInn
Copy link
Copy Markdown
Member

I'll take care of sqash.

@TheLarkInn TheLarkInn merged commit 11c2865 into webpack:master Jan 2, 2017
@willmendesneto willmendesneto deleted the refactor-api-plugin branch January 2, 2017 22:12
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.

5 participants