Prepare package for bundling in Atom #640

BinaryMuse · 2017-04-04T15:07:03Z

There are a bunch of changes necessary to support snapshotting in Atom. The snapshot will walk the require graph and pull in all the files to the snapshot, and then execute them in a raw V8 context. This means we get no require.resolve, no console.*, no atom, etc. So we need to move usage of those types of things until they're actually needed.

Drop the semver check inside the main entry point
Get things like Emitter and Disposable from event-kit instead of atom
Defer the use of require.resolve until it's actually needed
Defer the creation of the GithubPackage instance until the package is activated (because we have no atom to use to pass items into the constructor)
Use getPackageRoot() strategy to launch bins
Fix or defer loading of packages that use unavailable APIs at require time

In conjunction with atom/atom#14131

Fixes #618

BinaryMuse · 2017-04-05T06:35:38Z

lib/models/github-login-model.js

    return instance;
  }

-  constructor(Strategy = strategies.find(strat => strat.isValid())) {


Moving this check to the first time we actually try to use the strategy saves around ~100ms on the package's initial render (called from activate).

Another option might be to default to the real Keytar strategy and toggle to the shelling-out one with an environment variable or a (hidden?) config setting. Annoying for those of us who run master, but it'd keep us from paying any performance penalty for the common case of being bundled in a proper Atom distro.

This ensures that the instance is available before any consume* or deserialize* hooks are called

BinaryMuse · 2017-04-06T08:11:06Z

This now requires only the following additions to the snapshot creation script in atom/atom when we're ready to bundle:

diff --git a/script/lib/generate-startup-snapshot.js b/script/lib/generate-startup-snapshot.js
index 26761fc69..9eaa5f59c 100644
--- a/script/lib/generate-startup-snapshot.js
+++ b/script/lib/generate-startup-snapshot.js
@@ -27,6 +27,7 @@ module.exports = function (packagedAppPath) {
         modulePath.endsWith('.node') ||
         coreModules.has(modulePath) ||
         (relativePath.startsWith(path.join('..', 'src')) && relativePath.endsWith('-element.js')) ||
+        relativePath.startsWith(path.join('..', 'node_modules', 'dugite')) ||
         relativePath == path.join('..', 'exports', 'atom.js') ||
         relativePath == path.join('..', 'src', 'electron-shims.js') ||
         relativePath == path.join('..', 'src', 'safe-clipboard.js') ||
@@ -37,6 +38,7 @@ module.exports = function (packagedAppPath) {
         relativePath == path.join('..', 'node_modules', 'cson-parser', 'node_modules', 'coffee-script', 'lib', 'coffee-script', 'register.js') ||
         relativePath == path.join('..', 'node_modules', 'decompress-zip', 'lib', 'decompress-zip.js') ||
         relativePath == path.join('..', 'node_modules', 'debug', 'node.js') ||
+        relativePath == path.join('..', 'node_modules', 'fs-extra', 'lib', 'index.js') ||
         relativePath == path.join('..', 'node_modules', 'git-utils', 'lib', 'git.js') ||
         relativePath == path.join('..', 'node_modules', 'glob', 'glob.js') ||
         relativePath == path.join('..', 'node_modules', 'graceful-fs', 'graceful-fs.js') ||
@@ -47,6 +49,8 @@ module.exports = function (packagedAppPath) {
         relativePath == path.join('..', 'node_modules', 'less', 'lib', 'less-node', 'index.js') ||
         relativePath == path.join('..', 'node_modules', 'less', 'node_modules', 'graceful-fs', 'graceful-fs.js') ||
         relativePath == path.join('..', 'node_modules', 'minimatch', 'minimatch.js') ||
+        relativePath == path.join('..', 'node_modules', 'node-fetch', 'lib', 'fetch-error.js') ||
+        relativePath == path.join('..', 'node_modules', 'nsfw', 'node_modules', 'fs-extra', 'lib', 'index.js') ||
         relativePath == path.join('..', 'node_modules', 'superstring', 'index.js') ||
         relativePath == path.join('..', 'node_modules', 'oniguruma', 'lib', 'oniguruma.js') ||
         relativePath == path.join('..', 'node_modules', 'request', 'index.js') ||

smashwilson

🎉

smashwilson · 2017-04-06T13:45:52Z

lib/controllers/file-patch-controller.js

 import React from 'react';

-import {Point, Emitter} from 'atom';
+import {Point} from 'atom';


Huh, this import still works? I thought you'd meant that none of the Atom imports would work properly when bundled. Does it just have to do with the timing of when Emitter and CompositeDisposable are installed in the Atom global?

We use Point after activation, so it works fine. We had to bring in event-kit for Emitter and friends in other places, so it made sense to replace it package-wide.

smashwilson · 2017-04-06T13:51:49Z

lib/git-shell-out-strategy.js

+import {CompositeDisposable} from 'event-kit';

-import {GitProcess} from 'git-kitchen-sink';
+import {GitProcess} from 'dugite';


👍 Thanks for getting this upgrade in here 😄

smashwilson · 2017-04-06T13:53:26Z

lib/helpers.js

+  }
+
+  return DUGITE_PATH;
+}


👍 cool, cool

smashwilson · 2017-04-06T13:53:59Z

lib/helpers.js

 }

-export function deleteFileOrFolder(path) {
+export function deleteFileOrFolder(fileOrFolder) {


😆 I always do this. "path" is such a natural name to use as a variable...

smashwilson · 2017-04-06T13:57:52Z

lib/models/github-login-model.js

    return instance;
  }

-  constructor(Strategy = strategies.find(strat => strat.isValid())) {


Another option might be to default to the real Keytar strategy and toggle to the shelling-out one with an environment variable or a (hidden?) config setting. Annoying for those of us who run master, but it'd keep us from paying any performance penalty for the common case of being bundled in a proper Atom distro.

smashwilson · 2017-04-06T13:58:58Z

lib/views/commit-view.js

      }
    } else {
-      return this.props.maximumCharacterLimit;
+      return this.props.maximumCharacterLimit || '';


Michelle Tilley added 2 commits April 3, 2017 23:52

Simplify startup path

f89dbb6

Fix bad require

4c9fafa

BinaryMuse added the work-in-progress label Apr 4, 2017

BinaryMuse self-assigned this Apr 4, 2017

Michelle Tilley added 6 commits April 4, 2017 08:35

Defer creation of the GithubPackage object until activation

a0e9951

Import things from event-kit

517e22e

🎨

84d82e7

Use getPackageRoot() for locating bins

00de30b

Merge branch 'master' into mkt-ku-simplify-package-startup

fb996e6

🔥 unnecessary deserializer registration

d6484f4

BinaryMuse force-pushed the mkt-ku-simplify-package-startup branch from 331a1c2 to d14450f Compare April 4, 2017 19:53

Michelle Tilley added 9 commits April 4, 2017 12:54

git-kitchen-sink -> dugite

30b9b95

⬆️ etch@0.12.0

d14450f

Where in the world is Dugite

4266655

✅ fix failing tests

75c05e4

Add console.logs (that's a first)

29cbf12

Extract PrInfoByBranchRoute into routes

64c822d

We don't need these any more 🔥

2120a10

Merge branch 'master' into mkt-ku-simplify-package-startup

36217ef

Defer GithubLoginModel strategy validation test

d1ffecd

BinaryMuse commented Apr 5, 2017

View reviewed changes

smashwilson mentioned this pull request Apr 5, 2017

Instrument operation durations #642

Merged

BinaryMuse force-pushed the mkt-ku-simplify-package-startup branch from cc625f9 to 9ad8629 Compare April 6, 2017 07:56

Create GithubPackage instance in initialize package hook

9ad8629

This ensures that the instance is available before any consume* or deserialize* hooks are called

BinaryMuse requested review from kuychaco and smashwilson April 6, 2017 08:14

BinaryMuse removed the work-in-progress label Apr 6, 2017

smashwilson approved these changes Apr 6, 2017

View reviewed changes

Merge branch 'master' into mkt-ku-simplify-package-startup

18e3d7e

Merge branch 'master' into mkt-ku-simplify-package-startup

5af28e2

BinaryMuse merged commit 06c35e9 into master Apr 11, 2017

BinaryMuse deleted the mkt-ku-simplify-package-startup branch April 11, 2017 04:23

BinaryMuse mentioned this pull request Apr 12, 2017

Unable to push on Windows #652

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prepare package for bundling in Atom #640

Prepare package for bundling in Atom #640

Uh oh!

BinaryMuse commented Apr 4, 2017 •

edited

Loading

Uh oh!

BinaryMuse Apr 5, 2017

Uh oh!

smashwilson Apr 6, 2017

Uh oh!

BinaryMuse commented Apr 6, 2017

Uh oh!

smashwilson left a comment

Uh oh!

smashwilson Apr 6, 2017

Uh oh!

BinaryMuse Apr 11, 2017

Uh oh!

smashwilson Apr 6, 2017

Uh oh!

smashwilson Apr 6, 2017

Uh oh!

smashwilson Apr 6, 2017

Uh oh!

smashwilson Apr 6, 2017

Uh oh!

smashwilson Apr 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Prepare package for bundling in Atom #640

Prepare package for bundling in Atom #640

Uh oh!

Conversation

BinaryMuse commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BinaryMuse commented Apr 6, 2017

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BinaryMuse commented Apr 4, 2017 •

edited

Loading