Skip to content

refactor harmonyAcceptDependency to es6#3708

Merged
TheLarkInn merged 2 commits into
webpack:masterfrom
timse:refactor-harmony-accept-dependency-to-es6
Jan 4, 2017
Merged

refactor harmonyAcceptDependency to es6#3708
TheLarkInn merged 2 commits into
webpack:masterfrom
timse:refactor-harmony-accept-dependency-to-es6

Conversation

@timse

@timse timse commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

refactor

Did you add tests for your changes?

current tests should pass

If relevant, link to documentation update:

N/A

Summary

upgrade HarmonyAcceptDependency and HarmonyAcceptDependencyTemplate to es6

Does this PR introduce a breaking change?

No

source.insert(dep.range[1], ")(__WEBPACK_OUTDATED_DEPENDENCIES__); }");
} else {
source.insert(dep.range[1] - 0.5, ", function() { " + content + "}");
source.insert(dep.range[1] - 0.5, `, function() { ${content}}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sokra won't something break if space will be added before closing bracket?

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.

you probably need to update the stats test, but elsewise it should be fine.

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.

not sure, which space are you guys referring to? or should i add one here?

@chicoxyzzy chicoxyzzy Jan 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@timse I mean changing this line to

source.insert(dep.range[1] - 0.5, `, function() { ${content} }`)

for readability. Notice space before function's body closing curly bracket

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 should i add it?
will do then tonight!

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.

Yeah that looks great, I think that is a good approach.

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 :)

@chicoxyzzy chicoxyzzy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@timse timse mentioned this pull request Jan 4, 2017
6 tasks

@TheLarkInn TheLarkInn left a comment

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.

Nice work. Getting closer to Null/Const/Dependency.js ;-)

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.

4 participants