Ship yarn with hyper #381

sheerun · 2016-07-24T09:30:45Z

This is a step towards #282 and makes sure hyperterm always
installs with the same npm version it shipped with.

It also drops dependency on system's node.

rauchg · 2016-07-24T18:00:39Z

That's really sweet. I wonder how much bigger it makes our compressed builds / startup time ?

leo · 2016-07-25T20:11:13Z

Keep this in mind.

sheerun · 2016-07-25T20:24:31Z

It doesn't mater as you embed npm, and have can fix working version if you need.

Spawning a new process simply doesn't work because there's no node installed in electron app..

rauchg · 2016-07-26T23:43:12Z

I would love for @othiym23 to re-visit that decision. It's kind of a bummer that we need to ask the user to install node to use npm as an API :(

sindresorhus · 2016-07-27T11:27:39Z

Spawning a new process simply doesn't work because there's no node installed in electron app..

You can. Just spawn process.execPath with the ELECTRON_RUN_AS_NODE environment variable, and it will use the embedded Node.js.

childProcess.execSync(process.execPath, ['npm', 'install'], {env: {ELECTRON_RUN_AS_NODE: true}});

sheerun · 2016-07-27T16:54:58Z

I tried to implement it as @sindresorhus proposed, but for some reason electron refuses to exit when running npm with it. Here's how to reproduce:

export CWD=$(pwd) && (cd ~/.hyperterm_plugins && ELECTRON_RUN_AS_NODE=1 $CWD/node_modules/electron-prebuilt/dist/Electron.app/Contents/MacOS/Electron $CWD/node_modules/.bin/npm install --verbose)

For now it's blocking me, so any help would be appreciated..

rauchg · 2016-07-27T22:24:36Z

@sheerun seems like something we should be reporting to the electron team. This is looking very good btw, thanks @sindresorhus

rauchg · 2016-09-19T06:34:51Z

@sheerun I'd love to merge this. Do you have any news about whether the issue with exiting was fixed? Or any workarounds?

sheerun · 2016-09-19T14:09:46Z

The issue to fix is here: electron/electron#4944

With newest electron it kinda works, but takes 30 seconds to exit.

The best I can do is to rebase, catch.

sheerun · 2016-10-23T17:23:13Z

@leo @rauchg I've replaced npm with yarn and it runs like wonder. It's good to merge.

There's issue with CI build as xo doesn't recognize queue package as dependency (it is). Maybe that's something @sindresorhus could check..

matheuss · 2016-10-23T17:30:37Z

@sheerun queue is on the wrong package.json 😅 You need to move it to app/package.json 😄

sheerun · 2016-10-23T17:39:06Z

@matheuss Is yarn on correct one?

sheerun · 2016-10-23T17:56:20Z

Anyway, I fixed the issue with build. Ready for review and merge..

sheerun · 2016-11-20T11:47:16Z

@matheuss This is no longer in-progress. This is done.

matheuss · 2016-11-20T17:56:49Z

@sheerun thanks for the ping 😄

Is yarn on correct one?

Nope, you need to move it to the other one 👌

Could you please rebase to fix the merge conflicts? I'll make sure to discuss it with the team 👌

sheerun · 2016-11-20T20:09:56Z

@matheuss @rauchg Done. I hope I've rebased it the last time... :)

rauchg · 2016-11-21T09:50:55Z

This is fantastic!

On Sun, Nov 20, 2016 at 5:09 PM Adam Stankiewicz notifications@github.com
wrote:

@matheuss https://github.com/matheuss @rauchg
https://github.com/rauchg Done. I hope I've rebased it the last time...
:)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#381 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAy8Us6bM5zNRj3eHTzAU1UIsqEwQ6kks5rAKkVgaJpZM4JTiip
.

chabou · 2017-06-18T09:09:15Z

It is finished! It only needs some testing on Windows/Linux! But I'm struggling with Win10 installation on BootCamp 😞

albinekb · 2017-06-18T18:43:17Z

the app/bin/yarn-standalone.js should be built in the build step, it shouldn't be checked into git

but ok for now, i can PR that

chabou · 2017-06-18T22:16:57Z

Why? I consider yarn as an external program. If it was written in C++, we wouldn't want to compile it by ourself. What are the pros to not have this external dependency "compiled" and versioned in our git?

chabou · 2017-06-24T00:00:39Z

Problem with production build on Windows solved (bad yarn.lock file).
Tested on Mac and Windows (dev and prod build).

Need a test on Linux

chabou · 2017-06-26T21:05:44Z

Tested.

Ready to be merged

ppot · 2017-06-27T23:59:49Z

app/plugins.js

+
+      cp.exec(fullcmd, {
+        cwd: path,
+        env,


What happend when isDev === true since you are forcing env to be production

This production flag is for yarn. See https://yarnpkg.com/lang/en/docs/cli/install/#toc-yarn-install-production
No side effect to Hyper.

ppot · 2017-06-28T00:01:37Z

app/plugins.js

+        cwd: path,
+        env,
+        shell: true,
+        timeout: 1000 * 60 * 5,


add comment for // 5min

ms('5m') will be better. Thank you to have pointed this

ppot · 2017-06-28T00:03:00Z

app/plugins.js

-    }
+  function yarn(args, cb) {
+    spawnQueue.push(end => {
+      const fullcmd = [process.execPath, yarnPath].concat(args).join(' ');


fullcmd ? Simply use cmd or command

ppot · 2017-06-28T00:06:18Z

app/plugins.js

@@ -228,46 +223,47 @@ function toDependencies(plugins) {
 }

 function install(fn) {
-  const {shell: cfgShell, npmRegistry} = exports.getDecoratedConfig();
+  const yarnPath = resolve(__dirname, '..', 'bin', 'yarn-standalone.js');


yarnPath is not used elsewhere then inside yarn function. Better to move it inside function scope.

I will move it (envconst too)

ppot · 2017-06-28T00:07:42Z

app/plugins.js

+        timeout: 1000 * 60 * 5,
+        stdio: ['ignore', 'ignore', 'inherit']
+      }, err => {
+        console.log('Done!');


Be more specific. What is Done! ? Plugins installation? Plugins update?

We don't need this log.

ppot · 2017-06-28T00:09:34Z

app/plugins.js

-const shellEnv = require('shell-env');
+const queue = require('queue');
+
+const spawnQueue = queue({concurrency: 1});


spawnQueue or yarnQueue

spawnQueue is better.

chabou · 2017-06-28T06:06:01Z

We should test plugins update too.

chabou · 2017-06-28T06:51:17Z

Plugins update is ok!

Stanzilla · 2017-07-20T03:16:31Z

Close Hyper
Edit config file by hand to add a new plugin
Launch Hyper

For me it first errors that it can't load the new plugin, then the plugin "update" kicks in, which then installs the new plugin and then everything is fine. I just think that first error should be something we should avoid.

chabou · 2017-08-12T11:03:07Z

@Stanzilla this error is currently shown without my PR.
It could be better handled but it should be another PR.

chabou · 2017-08-14T07:49:29Z

FTR:
yarn v0.24.6 is embedded and extraResources feature of electron-builder is used to ship it in our bundle.

sheerun mentioned this pull request Jul 24, 2016

Install hpm by default #282

Closed

albinekb mentioned this pull request Jul 27, 2016

Use configured shell to update plugins #447

Merged

sheerun force-pushed the embednpm branch from eb73dcc to 09c9e65 Compare July 27, 2016 13:25

sheerun force-pushed the embednpm branch 2 times, most recently from e16afd8 to e436b9c Compare July 27, 2016 17:25

timothyis added the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Jul 29, 2016

mkscrg mentioned this pull request Aug 1, 2016

Feature request: Don't require the user to have npm installed (for plugins) #134

Closed

sheerun force-pushed the embednpm branch from e436b9c to bc7897c Compare September 19, 2016 14:09

sheerun force-pushed the embednpm branch 2 times, most recently from 6ccb764 to 90b5303 Compare October 23, 2016 17:20

sheerun force-pushed the embednpm branch from 90b5303 to 1a12932 Compare October 23, 2016 17:44

Ship npm@3 with hyperterm

fde5b1f

sheerun force-pushed the embednpm branch from 1a12932 to fde5b1f Compare November 20, 2016 20:09

chabou removed the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Jun 18, 2017

chabou mentioned this pull request Jun 18, 2017

Plugins not installing #191

Closed

chabou added the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Jun 18, 2017

chabou added 3 commits June 24, 2017 00:21

Merge branch 'master' into sheerun-embednpm

7861869

Fix yarn.lock

f35f4e4

Use extraResources instead of asarUnpack

a44e521

chabou removed the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Jun 24, 2017

ppot reviewed Jun 27, 2017

View reviewed changes

ppot reviewed Jun 28, 2017

View reviewed changes

Review changes

572c767

chabou mentioned this pull request Jul 4, 2017

Cannot install plugins #1976

Closed

2 tasks

Merge branch 'master' into sheerun-embednpm

ef070bd

ppot approved these changes Aug 14, 2017

View reviewed changes

chabou merged commit cd1b8cd into vercel:master Aug 14, 2017

ppot mentioned this pull request Aug 17, 2017

info about missing npm is only short visible #837

Closed

gschier mentioned this pull request Nov 4, 2017

Use Standalone Yarn for Plugins instead of NPM Kong/insomnia#569

Merged

Ship yarn with hyper #381

Ship yarn with hyper #381

Conversation

sheerun commented Jul 24, 2016

rauchg commented Jul 24, 2016

leo commented Jul 25, 2016 • edited Loading

sheerun commented Jul 25, 2016

rauchg commented Jul 26, 2016

sindresorhus commented Jul 27, 2016

sheerun commented Jul 27, 2016

rauchg commented Jul 27, 2016

rauchg commented Sep 19, 2016

sheerun commented Sep 19, 2016

sheerun commented Oct 23, 2016

matheuss commented Oct 23, 2016

sheerun commented Oct 23, 2016

sheerun commented Oct 23, 2016

sheerun commented Nov 20, 2016

matheuss commented Nov 20, 2016

sheerun commented Nov 20, 2016

rauchg commented Nov 21, 2016

chabou commented Jun 18, 2017

albinekb commented Jun 18, 2017 • edited Loading

chabou commented Jun 18, 2017

chabou commented Jun 24, 2017

chabou commented Jun 26, 2017

ppot Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chabou commented Jun 28, 2017

chabou commented Jun 28, 2017

Stanzilla commented Jul 20, 2017

chabou commented Aug 12, 2017

chabou commented Aug 14, 2017

leo commented Jul 25, 2016 •

edited

Loading

albinekb commented Jun 18, 2017 •

edited

Loading

ppot Jun 27, 2017 •

edited

Loading