Skip to content

Fix tarball file - #3221

Merged
bestander merged 3 commits into
yarnpkg:masterfrom
arcanis:fix-tarball-file
Apr 26, 2017
Merged

bestander merged 3 commits into
yarnpkg:masterfrom
arcanis:fix-tarball-file

Conversation

@arcanis

@arcanis arcanis commented Apr 21, 2017 •

Copy link
Copy Markdown
Member

Summary

The offline mirror feature accidentally disabled the codepath that was used for installing packages from file: protocol. The tests didn't caught this error because they're using a wrapper class, LocalTarballFetcher, that wasn't affected by this issue.

@arcanis
arcanis requested a review from bestander April 21, 2017 14:03
@arcanis

arcanis commented Apr 21, 2017

Copy link
Copy Markdown
Member Author

Wait, the revert shouldn't be there, I'll strip it

@arcanis

arcanis commented Apr 21, 2017

Copy link
Copy Markdown
Member Author

👍

@bestander

Copy link
Copy Markdown
Member

Looks like a few tests failed

@arcanis

arcanis commented Apr 25, 2017

Copy link
Copy Markdown
Member Author

Should be fine now

Comment thread __tests__/commands/install/lockfiles.js Outdated

jasmine.DEFAULT_TIMEOUT_INTERVAL = 150000;

test.concurrent = test.skip;

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.

oops :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

😱

@bestander

Copy link
Copy Markdown
Member

@arcanis, you forgot to reenable tests

@bestander
bestander merged commit d54fff3 into yarnpkg:master Apr 26, 2017
bestander pushed a commit that referenced this pull request May 2, 2017
* Fixes #3168

* Adds tests

* Fixes linting
@DanBuild DanBuild mentioned this pull request May 2, 2017
@bestander

Copy link
Copy Markdown
Member

This PR fixes relative paths but not absolute ones both on Windows and Unix.
Is it safe to add a root path to the regex, @arcanis?

@arcanis

arcanis commented May 3, 2017 •

Copy link
Copy Markdown
Member Author

I could change the regex to: /^(?=(?:\.{1,2}|[a-z]:)?[\\\/])/i, what do you think?

@arcanis

arcanis commented May 3, 2017

Copy link
Copy Markdown
Member Author

It seems to catch everything: https://regex101.com/r/zS5bmW/1

@bestander

Copy link
Copy Markdown
Member

Looks good to me, can you send a PR when you have a chance?

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