This repository has been archived by the owner. It is now read-only.

add "-offline" shorthand for "--cache-min 999999" #10331

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@lewiscowper
Contributor

lewiscowper commented Nov 10, 2015

I was going through @ashleygwilliams's slides and came across slide 15 where I discovered how to install a package from the cache, which is super helpful.

She suggested adding an alias of --offline, so I thought I'd explore the possibility that this PR would solve that problem. Although it does appear that it will actually shorthand to -offline which isn't quite what was suggested. Maybe there's another way of doing this to maintain the double dash syntax for full words that'd be better in this case, and if so, please let me know whether I can do this inside this PR.

Happy to close this if there's a totally different way of doing it, and I'm more than happy to add tests, but I couldn't find any other tests to test whether the shorthands expanded to the intended arguments, so I figured it's not seen as a useful unit test, which is reasonable.

@lewiscowper lewiscowper changed the title from add `-offline` shorthand for `--cache-min 999999` to add -offline shorthand for --cache-min 999999 Nov 10, 2015

@lewiscowper lewiscowper changed the title from add -offline shorthand for --cache-min 999999 to add "-offline" shorthand for "--cache-min 999999" Nov 10, 2015

@hemanth

This comment has been minimized.

Show comment
Hide comment
@hemanth

hemanth commented Nov 10, 2015

👍

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Nov 10, 2015

Contributor

I think you only need the second of the two changes – "shortcuts" are for genuine shortcuts. The piece that's missing is documentation, which is going to cause the tests to complain.

That saiiiiid, this is a nice thought, but @ashleygwilliams led you astray here, because as it stands this isn't actually going to guarantee offline behavior. npm will still try to connect to the registry if a package has dependencies on other packages that aren't in the cache. See also #8581 for another edge case that can arise when trying to use a high cache-min setting.

My concern is that landing this as-is would create confusion among users who don't understand the subtleties of how npm's cache works (which, going by this issue tracker, includes almost everybody). Instead of doing something that's going to cause a bunch more issues to be filed when people notice the "offline mode's" edge cases, I'd prefer to wait until full support for offline operation is added. This would entail making sure that npm doesn't generate any network traffic, and either errors or gives a strong warning that certain things aren't going to work when offline is set.

I leave this up to you, @lewiscowper – we don't like to leave PRs with open-ended tasks on them open for too long, but offline support would be very useful to a lot of people (at the very least, if you fixed #8581, you'd have the gratitude of @timoxley). Is this something you're interested in continuing to work on?

Contributor

othiym23 commented Nov 10, 2015

I think you only need the second of the two changes – "shortcuts" are for genuine shortcuts. The piece that's missing is documentation, which is going to cause the tests to complain.

That saiiiiid, this is a nice thought, but @ashleygwilliams led you astray here, because as it stands this isn't actually going to guarantee offline behavior. npm will still try to connect to the registry if a package has dependencies on other packages that aren't in the cache. See also #8581 for another edge case that can arise when trying to use a high cache-min setting.

My concern is that landing this as-is would create confusion among users who don't understand the subtleties of how npm's cache works (which, going by this issue tracker, includes almost everybody). Instead of doing something that's going to cause a bunch more issues to be filed when people notice the "offline mode's" edge cases, I'd prefer to wait until full support for offline operation is added. This would entail making sure that npm doesn't generate any network traffic, and either errors or gives a strong warning that certain things aren't going to work when offline is set.

I leave this up to you, @lewiscowper – we don't like to leave PRs with open-ended tasks on them open for too long, but offline support would be very useful to a lot of people (at the very least, if you fixed #8581, you'd have the gratitude of @timoxley). Is this something you're interested in continuing to work on?

@lewiscowper

This comment has been minimized.

Show comment
Hide comment
@lewiscowper

lewiscowper Nov 10, 2015

Contributor

That might be a big ask given my lack of experience in this area.

But I'm more than willing to give it a go, if you had anyone cli-side that might want to mentor/be available for Qs that would also be really helpful.

I'd say give me around 2-3 weeks (I have a few talks to give in between now and then), and I'll try and explore it, and if I can't solve it by then (so the end of November), then we should shut the PR completely.

Contributor

lewiscowper commented Nov 10, 2015

That might be a big ask given my lack of experience in this area.

But I'm more than willing to give it a go, if you had anyone cli-side that might want to mentor/be available for Qs that would also be really helpful.

I'd say give me around 2-3 weeks (I have a few talks to give in between now and then), and I'll try and explore it, and if I can't solve it by then (so the end of November), then we should shut the PR completely.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Nov 10, 2015

Contributor

If you ask questions here, one of us can answer them, and some of us are generally available on #npm on Freenode (although we're generally only paying close attention to IRC during our work hours, which are roughly 8 a.m. to 6 p.m. US/Pacific time).

I'd say give me around 2-3 weeks (I have a few talks to give in between now and then), and I'll try and explore it, and if I can't solve it by then (so the end of November), then we should shut the PR completely.

That sounds reasonable! Thanks, Lewis!

Contributor

othiym23 commented Nov 10, 2015

If you ask questions here, one of us can answer them, and some of us are generally available on #npm on Freenode (although we're generally only paying close attention to IRC during our work hours, which are roughly 8 a.m. to 6 p.m. US/Pacific time).

I'd say give me around 2-3 weeks (I have a few talks to give in between now and then), and I'll try and explore it, and if I can't solve it by then (so the end of November), then we should shut the PR completely.

That sounds reasonable! Thanks, Lewis!

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Nov 10, 2015

Member

I have a little experimental branch that does something like this– it's an experiment because it lacks docs or tests and may or may not be complete, but it might be helpful in getting an idea as to what direction you might take:

https://github.com/npm/npm/tree/iarna/experiment/disable-networking

Member

iarna commented Nov 10, 2015

I have a little experimental branch that does something like this– it's an experiment because it lacks docs or tests and may or may not be complete, but it might be helpful in getting an idea as to what direction you might take:

https://github.com/npm/npm/tree/iarna/experiment/disable-networking

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Nov 10, 2015

Member

For instance, it returns ECONNREFUSED if networking is disabled, something new like maybe EOFFLINE would probably be more appropriate. (This would also need an entry in lib/utils/error-handler to give user friendly error messages.)

What this doesn't do is disable git clones, which a --offline probably should.

Member

iarna commented Nov 10, 2015

For instance, it returns ECONNREFUSED if networking is disabled, something new like maybe EOFFLINE would probably be more appropriate. (This would also need an entry in lib/utils/error-handler to give user friendly error messages.)

What this doesn't do is disable git clones, which a --offline probably should.

@lewiscowper

This comment has been minimized.

Show comment
Hide comment
@lewiscowper

lewiscowper Nov 11, 2015

Contributor

Hi @iarna. I'd had a look through the wiki, and found your experiments. Thanks so much for them (and for publicising their existence), they are nice little explorations into a specific thing. I'm going to have a look into them over the next few evenings.

Can I ask though, is there a preferred way to run a local version of npm itself? I'd be worried of breaking something in npm completely, and then it being linked to my global npm install and having to do some acrobatics to fix it.

Contributor

lewiscowper commented Nov 11, 2015

Hi @iarna. I'd had a look through the wiki, and found your experiments. Thanks so much for them (and for publicising their existence), they are nice little explorations into a specific thing. I'm going to have a look into them over the next few evenings.

Can I ask though, is there a preferred way to run a local version of npm itself? I'd be worried of breaking something in npm completely, and then it being linked to my global npm install and having to do some acrobatics to fix it.

@lewiscowper

This comment has been minimized.

Show comment
Hide comment
@lewiscowper

lewiscowper Nov 30, 2015

Contributor

19 days have passed, December is upon me, and I haven't had a change to mess around with this exploratory PR any more than I did 19 days ago. I will close this PR to keep your stats looking good, because the changes proposed here don't work for reasons @othiym23 explained above. If I ever manage to get to the point that I might be closer than @iarna to figuring it out, I'll open a PR with the changes for discussion. I don't however, expect that to be soon, due to time, knowledge, and reasons.

Thanks for your time, help, clarifications, and your continued work over at npm.. (I did manage to get the local version of npm cli running via make link, which is sort of in the docs, but because it looked different to the other guides, I skipped it. <<)

Contributor

lewiscowper commented Nov 30, 2015

19 days have passed, December is upon me, and I haven't had a change to mess around with this exploratory PR any more than I did 19 days ago. I will close this PR to keep your stats looking good, because the changes proposed here don't work for reasons @othiym23 explained above. If I ever manage to get to the point that I might be closer than @iarna to figuring it out, I'll open a PR with the changes for discussion. I don't however, expect that to be soon, due to time, knowledge, and reasons.

Thanks for your time, help, clarifications, and your continued work over at npm.. (I did manage to get the local version of npm cli running via make link, which is sort of in the docs, but because it looked different to the other guides, I skipped it. <<)

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Dec 1, 2015

Contributor

Thank you for the update and I look forward to finding another opportunity for you to contribute to npm, Lewis. 🙇

Contributor

othiym23 commented Dec 1, 2015

Thank you for the update and I look forward to finding another opportunity for you to contribute to npm, Lewis. 🙇

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Oct 22, 2016

Contributor

Any news on this?

Would make stuff like https://twitter.com/ReBeccaOrg/status/789569913111269376 unnecessary, right?

Contributor

SimenB commented Oct 22, 2016

Any news on this?

Would make stuff like https://twitter.com/ReBeccaOrg/status/789569913111269376 unnecessary, right?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.