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

Enable repo-local core packages in the 'packages' folder #17686

Merged
merged 17 commits into from Aug 21, 2018

Conversation

Projects
None yet
5 participants
@daviwil
Member

daviwil commented Jul 13, 2018

Description of the Change

This PR is the first step to implementing RFC 003, enabling core Atom packages to live inside of the packages folder of the Atom repo while still being treated as packages.

I originally created this PR with the about and welcome packages moved into the packages folder but I decided to split those out into separate PRs so that we can more closely follow the process described in RFC 003.

Alternate Designs

There is another possible design that does not require a change to apm and instead includes the packages folder into Atom's app package, but this approach has the following drawbacks:

  • More code needed to make these packages show up as bundled packages in Atom
  • Code changes are needed in Atom to include each individual package in the startup snapshot
  • We no longer have a list of core packages in packageDependencies that live inside of atom/atom

Ultimately, installing these local package dependencies into node_modules is a cleaner and more consistent experience to what we do today.

Why Should This Be In Core?

This enables some core packages to be moved back into atom/atom to remove the need for separate repositories.

Benefits

Some technical benefits of this approach:

  • Minimal code changes are necessary to move a core package under packages. Just copy the package's code into a folder under packages and then change Atom's package.json to use file:./packages/package-name in the version field of the package's packageDependencies entry.
  • A user can easily test changes to package code under packages just by running Atom in dev mode.
  • It's easy for an Atom developer to see which packages are repo-local just by looking at packageDependencies in package.json

See RFC 003 for more benefits.

Possible Drawbacks

One possible drawback is that the change to apm to support local package dependency paths doesn't yet have a way to determine that the package under packages and its "installed" copy under node_modules are the same and don't require another install. This is mainly due to the fact that we no longer change the version in the package's package.json file, so it's hard to know for certain whether the package code has changed between builds. See the comments in atom/apm#797 for more details.

On the other hand, no tarball has to be downloaded any longer, so npm's own internal caching of dependencies should render the time required to "install" the package relatively moot.

See RFC 003 for other possible drawbacks.

Verification Process

  • Building Atom causes packages under packages to be included in Atom's node_modules folder
  • Packages from packages show up as Core Packages in Atom's settings view
  • Running Atom in dev mode enables changes to packages under packages to be reflected in the next reload without rebuilding atom, no apm link required.
  • Running atom --dev with ATOM_DEV_RESOURCE_PATH set causes that path to be used as the dev resource path
  • Running atom --dev with no ATOM_DEV_RESOURCE_PATH set causes current directory to be used as dev resource path only if the current directory appears to contain an Atom repo
  • Running atom --dev with no ATOM_DEV_RESOURCE_PATH set in a directory that isn't an Atom repo causes the default Atom repo path ~/github/atom to be used

Applicable Issues

Tasks

@daviwil daviwil changed the title from WIP: Enable repo-local core packages in the 'packages' folder to Enable repo-local core packages in the 'packages' folder Aug 7, 2018

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Aug 9, 2018

Member

OK, this is ready for review!

My priority in this PR was to make the core package dev process from cloning atom/atom to running atom --dev as smooth as possible. This change enables the following workflow:

  1. Install Atom (at least a channel using Electron compatible with master)
  2. Clone atom/atom
  3. Run script/dev to install all dependencies and then launch Atom in dev mode

This removes the need for a contributor to need to do a full build of Atom or to understand how to set up ATOM_DEV_RESOURCE_PATH to run Atom in dev mode.

Would love to hear feedback on any part of this!

Member

daviwil commented Aug 9, 2018

OK, this is ready for review!

My priority in this PR was to make the core package dev process from cloning atom/atom to running atom --dev as smooth as possible. This change enables the following workflow:

  1. Install Atom (at least a channel using Electron compatible with master)
  2. Clone atom/atom
  3. Run script/dev to install all dependencies and then launch Atom in dev mode

This removes the need for a contributor to need to do a full build of Atom or to understand how to set up ATOM_DEV_RESOURCE_PATH to run Atom in dev mode.

Would love to hear feedback on any part of this!

Show outdated Hide outdated script/dev Outdated
@jasonrudolph

This comment has been minimized.

Show comment
Hide comment
@jasonrudolph

jasonrudolph Aug 9, 2018

Member

Running Atom in dev mode enables changes to packages under packages to be reflected in the next reload without rebuilding atom, no apm link required.

Member

jasonrudolph commented Aug 9, 2018

Running Atom in dev mode enables changes to packages under packages to be reflected in the next reload without rebuilding atom, no apm link required.

@jasonrudolph

I've noted a few questions and suggestions inline below. I hope this helps!

Show outdated Hide outdated script/dev Outdated
Show outdated Hide outdated script/dev Outdated
Show outdated Hide outdated script/dev Outdated
Show outdated Hide outdated script/dev Outdated
Show outdated Hide outdated script/dev Outdated
Show outdated Hide outdated script/lib/run-apm-install.js Outdated
@maxbrunsfeld

Run script/dev to install all dependencies and then launch Atom in dev mode

This removes the need for a contributor to need to do a full build of Atom or to understand how to set up ATOM_DEV_RESOURCE_PATH to run Atom in dev mode.

In the future, when we have many packages in the packages folder, I would think installing dependencies would take significant time. Even now, redundant runs of script/bootstrap take 17 seconds on my laptop.

This seems like something I'd want to keep as an explicit separate step from the default way that I start Atom in development. Could we address the papercut about ATOM_DEV_RESOURCE_PATH by defaulting the devResourcePath to process.cwd() in the case where the environment variable isn't set and ~/github/atom (the current default) does not exist? Maybe we could check if you're currently in the atom repo by looking for $PWD/atom.sh or some other distinctive file.

@smashwilson

This is pretty cool 👍 I'm on board with this approach.

  • I'd like to see script/dev pass its arguments to the launched Atom instance. Otherwise we can't use this to iterate on Atom bugs that only manifest in non-Atom projects.
  • To address @maxbrunsfeld's point about bootstrapping taking so long: in addition to improving dev resource path autodetection, maybe we could only invoke it if scripts/node_modules/ or apm/node_modules/ are missing? That would preserve the goal of minimizing manual steps from fresh clone to development iteration, but also let us avoid redundant bootstrapping.
@@ -30,13 +30,15 @@ describe('PackageManager', () => {
expect(packageManger.packageDirPaths[0]).toBe(path.join(configDirPath, 'packages'))
})
it('adds regular package path and dev package path in dev mode', () => {
it('adds regular package path, dev package path, and Atom repo package path in dev mode and dev resource path is set', () => {

This comment has been minimized.

@smashwilson

smashwilson Aug 10, 2018

Member

Oxford comma 4 life, yo

@smashwilson

smashwilson Aug 10, 2018

Member

Oxford comma 4 life, yo

Show outdated Hide outdated script/dev Outdated
Show outdated Hide outdated script/dev Outdated
Show outdated Hide outdated script/dev Outdated
@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 10, 2018

Member

Even now, redundant runs of script/bootstrap take 17 seconds on my laptop.

Yikes. 😱

See, this seems like a problem to me regardless. script/bootstrap is invoked even if you try to run script/build --help. atom/apm#808 should help there but it sounds like we have more work to do 🔧 .

Member

smashwilson commented Aug 10, 2018

Even now, redundant runs of script/bootstrap take 17 seconds on my laptop.

Yikes. 😱

See, this seems like a problem to me regardless. script/bootstrap is invoked even if you try to run script/build --help. atom/apm#808 should help there but it sounds like we have more work to do 🔧 .

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 10, 2018

Contributor

script/bootstrap is invoked even if you try to run script/build --help

Ha, yeah that's pretty bad. We could avoid that just by explicitly special-casing --help though.

To address maxbrunsfeld's point about bootstrapping taking so long: in addition to improving dev resource path autodetection, maybe we could only invoke it if scripts/node_modules/ or apm/node_modules/ are missing? That would preserve the goal of minimizing manual steps from fresh clone to development iteration, but also let us avoid redundant bootstrapping.

I also think it's ok if people have to run script/bootstrap manually though. Overall, I think it's valuable to avoid unnecessary differences between production and development, and I question the value of bundling build and launch into one step.

Contributor

maxbrunsfeld commented Aug 10, 2018

script/bootstrap is invoked even if you try to run script/build --help

Ha, yeah that's pretty bad. We could avoid that just by explicitly special-casing --help though.

To address maxbrunsfeld's point about bootstrapping taking so long: in addition to improving dev resource path autodetection, maybe we could only invoke it if scripts/node_modules/ or apm/node_modules/ are missing? That would preserve the goal of minimizing manual steps from fresh clone to development iteration, but also let us avoid redundant bootstrapping.

I also think it's ok if people have to run script/bootstrap manually though. Overall, I think it's valuable to avoid unnecessary differences between production and development, and I question the value of bundling build and launch into one step.

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Aug 14, 2018

Member

@maxbrunsfeld and @smashwilson: Thanks for the feedback! Would it be better if I just got rid of script/dev completely and moved the package installation logic back into script/bootstrap? The first-time workflow for this would be:

  1. Install Atom Nightly (for best results)
  2. Clone the Atom repo
  3. Run script/bootstrap to install Atom dependencies and those of the packages under packages
  4. Run atom --dev .

The last step would be dependent on me implementing @maxbrunsfeld's suggestion to make Atom smarter about finding the ATOM_DEV_RESOURCE_PATH, which shouldn't be too hard. Might be able to look for package.json in process.cwd() and see if it contains "name": "atom" in the first 5 lines.

If the developer ever updated package dependencies in packages/somepackage they could just manually run apm install there or run script/bootstrap all over again.

What do you think?

Member

daviwil commented Aug 14, 2018

@maxbrunsfeld and @smashwilson: Thanks for the feedback! Would it be better if I just got rid of script/dev completely and moved the package installation logic back into script/bootstrap? The first-time workflow for this would be:

  1. Install Atom Nightly (for best results)
  2. Clone the Atom repo
  3. Run script/bootstrap to install Atom dependencies and those of the packages under packages
  4. Run atom --dev .

The last step would be dependent on me implementing @maxbrunsfeld's suggestion to make Atom smarter about finding the ATOM_DEV_RESOURCE_PATH, which shouldn't be too hard. Might be able to look for package.json in process.cwd() and see if it contains "name": "atom" in the first 5 lines.

If the developer ever updated package dependencies in packages/somepackage they could just manually run apm install there or run script/bootstrap all over again.

What do you think?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 14, 2018

Contributor

What do you think?

That seems like a good setup to me. But if anybody has strong feelings in favor of script/dev, I don't want to crush the idea.

Contributor

maxbrunsfeld commented Aug 14, 2018

What do you think?

That seems like a good setup to me. But if anybody has strong feelings in favor of script/dev, I don't want to crush the idea.

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Aug 21, 2018

Member

OK folks, I've responded to all the feedback here so I think this is ready for another pass. Summary of the changes:

  • Moved package bootstrapping back into script/bootstrap
  • Taught Atom how to look for an atom repo at process.cwd() and use it as the dev resource path when --dev is passed if ATOM_DEV_RESOURCE_PATH is not set
  • Deleted script/dev
  • Changed the stderrOnly param of runApmInstall to stdioOptions for more flexibility

Let me know if there's anything else you'd like to see!

Member

daviwil commented Aug 21, 2018

OK folks, I've responded to all the feedback here so I think this is ready for another pass. Summary of the changes:

  • Moved package bootstrapping back into script/bootstrap
  • Taught Atom how to look for an atom repo at process.cwd() and use it as the dev resource path when --dev is passed if ATOM_DEV_RESOURCE_PATH is not set
  • Deleted script/dev
  • Changed the stderrOnly param of runApmInstall to stdioOptions for more flexibility

Let me know if there's anything else you'd like to see!

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 21, 2018

Member

Approved

make it so

Member

smashwilson commented Aug 21, 2018

Approved

make it so

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Aug 21, 2018

Member

Thanks all! Merging it 🚢

Member

daviwil commented Aug 21, 2018

Thanks all! Merging it 🚢

@daviwil daviwil merged commit ad4553c into master Aug 21, 2018

3 checks passed

VSTS: Atom Pull Requests 20180821.4 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the dw-repo-local-core-packages branch Aug 21, 2018

@daviwil daviwil referenced this pull request Aug 21, 2018

Merged

Improve repo-local packages implementation #17892

5 of 5 tasks complete

@jasonrudolph jasonrudolph referenced this pull request Aug 23, 2018

Closed

Iteration Plan: August 20 - August 31 #17910

13 of 16 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment