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

Integration #3

Merged
merged 8 commits into from
May 3, 2017
Merged

Integration #3

merged 8 commits into from
May 3, 2017

Conversation

debopamsengupta
Copy link
Owner

Hi @matteofigus ,
I added some integration tests, to ensure full Windows support.

Let me know if you feel anything else might be important :)

@matteofigus
Copy link
Collaborator

Looks good! Need to find out what's going on with Windows. My assumption is that the require stuff fails due to some path normalisation that is not right for Windows.

Another thing to test, not sure it is supported out of the box, is the possibility to require different file trees, such as require('lodash/package.json') or require('lodash@version/package.json') which is something we worked on here opencomponents/oc#323

it('should fetch correct version', () => {
let requirer = new ry.Requirer();
let a = requirer.require('lodash@1.3.1');
expect(a.drop).toBeTruthy(); //for 1.x
Copy link
Owner Author

Choose a reason for hiding this comment

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

just noticed lodash has a VERSION property available 😂 will update the tests

@debopamsengupta
Copy link
Owner Author

@matteofigus Finally discovered the issue with windows , which is scott113341/npm-install-version#32

Will wait on that to get merged, or would it be better to rewrite the installation/requiring logic in this project ?

@matteofigus
Copy link
Collaborator

Let's see if they reply and merge. If they take too much to reply, probably we can fork or re-implement (it should be not too much code for that).

Copy link
Collaborator

@matteofigus matteofigus left a comment

Choose a reason for hiding this comment

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

🎉

@matteofigus matteofigus merged commit ba8cd06 into master May 3, 2017
@matteofigus matteofigus deleted the integration branch May 3, 2017 16:34
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