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

refactor Dependency.js to es6 #3696

Merged
merged 2 commits into from Jan 8, 2017

Conversation

timse
Copy link
Member

@timse timse commented Jan 3, 2017

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 Dependency.js to es6

Does this PR introduce a breaking change?

No

Other
Dont expose compareLocations as Dependency attribute anymore as its only used internally anyways

@timse
Copy link
Member Author

timse commented Jan 3, 2017

should the file be moved to /dependencies ?

@TheLarkInn
Copy link
Member

@timse so this one is a little more involved to switch over because it is a top level superclass there are multiple levels of inheritance between this one and any other file that ends in Dependency.

So to refactor this one, you would also need to first refactor any file that ends in Dependency. Good example is Dependency => NullDependency => ConstDependency. If your not quite up for that much refactoring that is okay, feel free to close this PR and work on the lower level (in the hierarchy) Dependency subclasses like anything in the dependency folder. Let me know if any of this is unclear.

@timse
Copy link
Member Author

timse commented Jan 3, 2017

i guess i need to start at the top of the foodchain here to make this work :)


Dependency.compareLocations = require("./compareLocations");
Copy link
Member

Choose a reason for hiding this comment

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

This is technically an API change; nothing left around that assumes it can find compareLocations in the constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope :)

@timse
Copy link
Member Author

timse commented Jan 6, 2017

now dependes on #3812 and #3818

@TheLarkInn TheLarkInn closed this Jan 8, 2017
@TheLarkInn TheLarkInn reopened this Jan 8, 2017
@TheLarkInn
Copy link
Member

Closed and reopened to kick off a new build.

@timse
Copy link
Member Author

timse commented Jan 8, 2017

checking why this fails!

dont expose compareLocations as Dependency attribute anymore as its only used internally anyways
its only consumer is the Compilation
maybe this should be inlined?
@timse timse force-pushed the refactor-dependency-to-es6 branch from b874c58 to 0b0932f Compare January 8, 2017 04:58
@timse
Copy link
Member Author

timse commented Jan 8, 2017

found it. The only user of Dependency.compare seams to be Compilation, maybe we can inline that actually?

@TheLarkInn
Copy link
Member

Well freaking done!!! That was a huge one.

@TheLarkInn TheLarkInn merged commit e6d3a40 into webpack:master Jan 8, 2017
@TheLarkInn
Copy link
Member

Wow thank you so much. Would you mind shooting me an email? You can find it on my profile page. I'd like to chat with you briefly <3

@timse timse deleted the refactor-dependency-to-es6 branch January 8, 2017 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants