Skip to content

fix issue #3252 - #3259

Merged
sokra merged 7 commits into
webpack:masterfrom
zhaoshengjun:issue-3252
Nov 9, 2016
Merged

sokra merged 7 commits into
webpack:masterfrom
zhaoshengjun:issue-3252

Conversation

@zhaoshengjun

Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

  • [x ] Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • [ x] Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Default argument assignment is missing. Ref #3252

What is the new behavior?
Default argument assignment will be correctly placed.

Does this PR introduce a breaking change?

  • Yes
  • [x ] No

If this PR contains a breaking change, please describe the following...

  • Impact:
  • Migration path for existing applications:
  • Github Issue(s) this is regarding:

Other information:

@zhaoshengjun

Copy link
Copy Markdown
Contributor Author

Test passed on node 7, but failed on node 6 due to yarn.

@@ -0,0 +1,7 @@
module.exports = function supportDefaultAssignment() {
try {
return true;

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 doesn't look correct.

@zhaoshengjun zhaoshengjun Nov 9, 2016

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.

I just want it to be filtered. Because it fails when use uglifyjs plugin.

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.

Comment thread lib/Parser.js Outdated
break;
default:
this.enterPattern(property.value, onIdent);
}

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.

Could you remove the whole switch-case and shorten this to this.enterPattern(property.value, onIdent)?

From AST explorer it looks like property.type is always Property.

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.

Sure, I will make the change.

@sokra
sokra merged commit 1a21859 into webpack:master Nov 9, 2016
@sokra

sokra commented Nov 9, 2016

Copy link
Copy Markdown
Member

Thanks

@zhaoshengjun

Copy link
Copy Markdown
Contributor Author

My pleasure!

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.

2 participants