Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

kuychaco
Copy link
Contributor

@kuychaco kuychaco commented Jun 1, 2016

Fixes #463
Fixes #322
Fixes #436

@BinaryMuse
Copy link

Fixes #463

@winstliu
Copy link
Contributor

winstliu commented Jun 2, 2016

As well as #322 and #436.

@kuychaco
Copy link
Contributor Author

kuychaco commented Jun 3, 2016

@atom/core @damieng @atom/feedback - this is ready for review 👀 , thoughts and feedback welcome!

@BinaryMuse
Copy link

BinaryMuse commented Jun 3, 2016

Okay, this uses a new installation strategy that we came up with with @kevinsawicki. Previously, we installed packages by creating a blank folder with a node_modules subfolder, and doing npm install /path/to/downloaded/package.tgz, which installed the apm package in the node_modules subfolder. We then grabbed this lone package folder and moved it into ~/.atom/packages.

          .-~~~-.
  .- ~ ~-(       )_ _
 /                     ~ -.
|         atom.io           \
 \                         .'
   ~- . _____________ . -~
             │                 ┌───────────────────────┐
             │             ┌──▶│      /tmp/folder      │
             │             │   └───────────────────────┘
         download          │      ┌───────────────────────┐
             │             │      │     node_modules      │
             ▼             │      └───────────────────────┘
    ┌────────────────┐     │          ┌───────────────────────┐
    │                │     │       ┌──│        package        │
    │  package.tgz   │     │       │  └───────────────────────┘
    │                │     │       │       ┌───────────────────────┐
    └───────┬────────┘     │       │       │     node_modules      │
            │              │       │       └───────────────────────┘
            │              │       │          ┌────┐  ┌────┐  ┌────┐
            │                      └────copy┐ │dep1│  │dep2│  │dep3│
            └── npm install package.tgz     │ └────┘  └────┘  └────┘
                                            │
                                            ▼
                                ┌───────────────────────┐
                                │   ~/.atom/packages/   │
                                └───────────────────────┘

With the new npm v3 package flattening mechanism, we now also get a large number of the package's dependencies at the same level, making it difficult to move the package and all its dependencies into ~/.atom.

          .-~~~-.                                              _____
  .- ~ ~-(       )_ _                                   ____  |____ |
 /                     ~ -.     _ __  _ __  _ __ ___   / __ \     / /
|         atom.io           \  | '_ \| '_ \| '_ ` _ \ / / _` |    \ \
 \                         .'  | | | | |_) | | | | | | | (_| |.___/ /
   ~- . _____________ . -~     |_| |_| .__/|_| |_| |_|\ \__,_|\____/
             │                       | |               \____/
             │                       |_|
             │
         download              ┌───────────────────────┐
             │             ┌──▶│      /tmp/folder      │
             ▼             │   └───────────────────────┘
    ┌────────────────┐     │      ┌───────────────────────┐
    │                │     │      │     node_modules      │
    │  package.tgz   │     │      └───────────────────────┘
    │                │     │         ┌────┐ ┌────┐ ┌────────────┐
    └───────┬────────┘     │         │dep1│ │dep2│ │  package   │
            │              │         └────┘ └────┘ └────────────┘
            │              │                         ┌─────────────┐
            │              │                         │node_modules │
            │                                        └─────────────┘
            └── npm install package.tgz                 ┌────┐
                                                        │dep3│
                                                        └────┘

It's not enough to just move all the other folders into the package's node_modules folder, because npm will only flatten packages if they don't conflict, so if my_package depended on underscore@1 and also depended on awesome_module which itself depended on underscore@2, we'd get conflicts when performing the move.

Instead, we now unpack the downloaded .tgz file from GitHub into a temporary folder and run npm install in it directly, then move that whole folder into ~/.atom. In order to maintain the same behavior (e.g. don't install dev dependencies, don't run prepublish hooks, etc.) we pass some flags to npm and also directly modify the package's scripts entries to literally remove certain scripts from the package.json if they have any. A test for this is included.

Additionally, since npm no longer installs the base .tgz file directly, it will not cache it — we've disabled the test that checks for this. It will, however, still cache all the files for all the dependencies, which we feel is the important part of this. We could write custom code to cache the apm packages as well, but it didn't feel very important to do so.

This feels pretty heavy handed, but seems to be working, and npm@3 brings some huge wins, so we'd love to get some 💭 around this, and on the caching decision. Thanks!

@as-cii
Copy link
Contributor

as-cii commented Jun 3, 2016

Thanks for taking this on, @BinaryMuse and @kuychaco! 👏 ❤️ 💯

Additionally, since npm no longer installs the base .tgz file directly, it will not cache it — we've disabled the test that checks for this.

Can we delete it entirely? We can put it back at some point in the future, in case things change.

This feels pretty heavy handed, but seems to be working, and npm@3 brings some huge wins, so we'd love to get some 💭 around this, and on the caching decision.

👍 on the caching decision: so long as package dependencies are cached, I think we're good.

I am a bit more concerned about removing lifecycle hook scripts, although I guess there's always a defined set of them and we should be fine as long as we remove them all.


Thinking more about this, I wonder if we could use a variation of the approach we're taking right now on master: the nice thing about it is that it lets npm manage all the complexity of package-related concerns, so that we just perform the copy task.

So, what about doing exactly the same thing we were doing before, but pass the --global-style argument to npm install? From the docs:

The --global-style argument will cause npm to install the package into your local node_modules folder with the same layout it uses with the global node_modules folder. Only your direct dependencies will show in node_modules and everything they depend on will be flattened in their node_modules folders. This obviously will elminate some deduping.

What this means is that we can just copy the folder, but still have the deduping happen on a per-package basis (which, if I understand correctly, seems to be what we would be doing with this pull-request approach too). Thoughts? 💭

@BinaryMuse
Copy link

BinaryMuse commented Jun 3, 2016

So despite the work put into the diagrams, they're simply not necessary because @as-cii seems to have tracked down the flag we were looking for through all the RTFM-ing we did yesterday. Not sure how we missed it. :) Looks like the flag turns off package flattening for the first-level of node_modules, which is precisely what we need. (Maybe instead of ASCII art I should have just made as-cii art :P)

@kuychaco Take a look at the new diff; if you're happy we can reset --hard and apply the two latest commits directly on top of 9d99f97.

@BinaryMuse BinaryMuse mentioned this pull request Jun 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants