Use external pathval module #830
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks good but agreed on waiting for pathval version fix before merging. |
shvaikalesh
reviewed
Oct 5, 2016
| * Main exports | ||
| */ | ||
|
|
||
| var exports = module.exports = {}; |
There was a problem hiding this comment.
Am I missing something or this line is extra?
There was a problem hiding this comment.
You are totally right! I messed up this file on the latest rebase.
I have just fixed this.
lucasfcosta
force-pushed
the
external-pathval
branch
from
56dac5d
to
575c001
Compare
|
-318 lines 馃槑 |
|
LGTM! Great work @lucasfcosta |
|
@lucasfcosta As mentioned in chaijs/pathval#39 (comment), I think that we should manually update pathval to 1.0.0 and then update this PR to use that. |
lucasfcosta
force-pushed
the
external-pathval
branch
from
575c001
to
4968213
Compare
|
This one is done! I also added the following note to our migration guide (just in case):
|
lucasfcosta
force-pushed
the
external-pathval
branch
from
4968213
to
206c306
Compare
lucasfcosta
force-pushed
the
external-pathval
branch
from
206c306
to
a33ab48
Compare
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This includes a refactor for Chai to use the external
pathvalmodule as we discussed previously on #457 and #737.I also removed the old tests since they're now running at the separate module and removed an unused utility export (
.getPathValue), if you guys want me to keep it just tell me, but since we were not using it I just removed that, which is also a breaking change if anyone is usingutils.getPathValue. If you disagree let me know, I'd love to hear your opinions on this matter.Also, since we needed
pathval's exports twice I added a new declaration at the top of ourutils/index.jsfile for that, I thought it would be better to do it this way than thrusting Node would use its cache correctly to find the same module. If you think there's a better way to do this, share your ideas 馃槃At last, but not least, please take a look at this comment before merging this PR since the latest release of
pathvalhas breaking changes but we just bumped theminorversion number, so if we decide to release a newmajorversion I'll have to changepackage.json.Thanks guys! 鉂わ笍