Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Update github to 0.4.2 #15191

Merged
merged 3 commits into from
Aug 8, 2017
Merged

Conversation

smashwilson
Copy link
Contributor

Doing this in a PR because it broke snapshotting, likely because 0.4.1 changes the way that dependencies are de-duped.

@smashwilson
Copy link
Contributor Author

Looks like this depends on atom/electron-link#6 because we aren't transpiling async/await away any more. ⬆️ ⬆️ ⬆️

@smashwilson
Copy link
Contributor Author

Hmm:

Verifying if snapshot can be executed via `mksnapshot`
/Users/distiller/atom/out/startup.js:48387
        async activate() {
              ^^^^^^^^
SyntaxError: Unexpected identifier
  at Object.exports.runInNewContext (vm.js:71:16)
  at electronLink.then (/Users/distiller/atom/script/lib/generate-startup-snapshot.js:79:8)

@as-cii, any idea what could cause this with snapshotting? Do we not support async/await in snapshot scripts, even though it works natively in Electron... ?

@smashwilson smashwilson changed the title Update github to 0.4.1 Update github to 0.4.2 Aug 8, 2017
@smashwilson smashwilson merged commit 1b22c59 into atom:master Aug 8, 2017
@smashwilson smashwilson deleted the aw-upgrade-github branch August 8, 2017 23:45
@smashwilson
Copy link
Contributor Author

@as-cii, any idea what could cause this with snapshotting? Do we not support async/await in snapshot scripts, even though it works natively in Electron... ?

As a follow-up here: my understanding is that vm.runInNewContext() is used as a quick syntax check on the electron-link modified snapshot.js source, which uses the existing node that's running script/build. On CI that's a pre-async/await node. @BinaryMuse suggested using an electron process withELECTRON_RUN_AS_NODE to do the check with the actual electron v8 version. Think that'd be okay, or is there a chance that the electron-mksnapshot would fail when that passed... ?

@as-cii
Copy link
Contributor

as-cii commented Aug 9, 2017

Think that'd be okay, or is there a chance that the electron-mksnapshot would fail when that passed... ?

Yes, that's a really good idea: I think the version we use to perform the check should match the version of Node Atom ships with. 👍

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.

2 participants