feat(generic): allow specifying electron-prebuilt-compile via URL #450

jacobq · 2018-03-12T21:20:40Z

I have read the contribution documentation for this project.
I agree to follow the code of conduct that this project follows, as appropriate.
The changes are appropriately documented (if applicable).
The changes have sufficient test coverage (if applicable).
The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Replace home-made RegEx for testing whether or not the version string of electron-prebuild-compile with exact-version.
- Since this functionality is not closely related to electron / electron-forge, I think it makes sense to use and external library instead. Even though it is relatively small, there are a lot of subtleties that should have test coverage.
- The existing implementation does not correctly identify strings like 1.x, 1.7.0 || 1.8.3, or 1.0.0 - 1.5.0 as a range.
Obtain electron version via new getElectronVersion util, which reads the electron-prebuilt-compile's package.json (using the existing util readPackageJSON), which is guaranteed (by package.json / npm publish semantics) to be in a predictable format.

Why? One reason is that it should make it much easier to try the latest version of electron before a corresponding version of the official electron-prebuilt-compile module is published to NPM.

malept

~~Out of curiosity, what's the use case that necessitated this PR?~~ Never mind, didn't finish reading the summary.

fix(generic): allow specifying electron-prebuilt-compile via URL

I think this is a feature, not a bugfix.

malept · 2018-03-12T21:27:32Z

.gitignore

@@ -4,3 +4,6 @@ docs
 .nyc_output
 coverage
 npm-debug.log
+
+# Prevent adding test fixture package-lock files from npm@5 
+test/**/package-lock.json


I'd say just ignore all package-lock.json files.

Just curious, is the reason why electron-forge isn't using package-lock.json or yarn.lock to control dependency upgrades because consumer won't use them anyway (so by not having them we should detect potential breaks earlier)?

We're going to require Yarn (well, technically bolt) in v6.

malept · 2018-03-12T21:29:35Z

src/util/get-electron-version.js

+import readPackageJSON from './read-package-json';
+
+export default async (projectDir) => {
+  const epcPackageJSON = await readPackageJSON(path.resolve(projectDir, 'node_modules', 'electron-prebuilt-compile'));


Just call it packageJSON, we're going to decouple electron-compile from Forge.

malept · 2018-03-12T21:31:56Z

src/util/get-electron-version.js

+import path from 'path';
+import readPackageJSON from './read-package-json';
+
+export default async (projectDir) => {


@MarshallOfSound we're going to need to figure out how to merge this with https://github.com/electron-userland/electron-forge/pull/432/files#diff-f5dc721cced1da6c967b89a0309237de

malept · 2018-03-12T21:37:00Z

src/api/make.js

@@ -129,7 +130,8 @@ export default async (providedOptions = {}) => {

  await runHook(forgeConfig, 'preMake');

-  for (const targetArch of parseArchs(platform, arch, packageJSON.devDependencies['electron-prebuilt-compile'])) {
+  const electronVersion = getElectronVersion(dir);


malept · 2018-03-12T21:37:13Z

src/api/start.js

@@ -51,8 +52,9 @@ export default async (providedOptions = {}) => {
  }

  const forgeConfig = await getForgeConfig(dir);
+  const electronVersion = getElectronVersion(dir);


jacobq · 2018-03-12T21:47:52Z

@malept Oops, I was pushing changes while you were reviewing, sorry. Feel free to edit/fix/squash as needed (you should be able to write to this branch) or to let me know if you want me to make any changes myself. Thanks for checking it out!

jacobq · 2018-03-12T21:48:37Z

Oops, I'll add those missing awaits right now (originally I had it as a sync function).

jacobq · 2018-03-12T22:19:46Z

@malept I've revised, rebased, and pushed. Please take another look when you have the chance.

malept · 2018-03-12T22:25:16Z

.gitignore

@@ -4,3 +4,6 @@ docs
 .nyc_output
 coverage
 npm-debug.log
+package-lock.json
+yarn.lock


I don't know if we want to add yarn.lock into .gitignore right now given that it's going to be committed in v6.

malept · 2018-03-12T22:26:52Z

src/util/get-electron-version.js

+export default async (projectDir) => {
+  let result = null;
+
+  const modulesToExamine = ['electron-prebuilt-compile', 'electron-compile', 'electron'];


['electron-prebuilt-compile', 'electron', 'electron-prebuilt']

malept · 2018-03-12T22:27:39Z

src/util/get-electron-version.js

+  let result = null;
+
+  const modulesToExamine = ['electron-prebuilt-compile', 'electron-compile', 'electron'];
+  for (let i = 0; i < modulesToExamine.length; i += 1) {


for...of?

Also @MarshallOfSound this would be interesting if we gather this info when flora-colossus walks the module tree.

malept · 2018-03-12T22:28:23Z

src/util/get-electron-version.js

+      result = packageJSON.version;
+      break;
+    } catch (e) {
+      d(`Could not read package.json for moduleName=${moduleName}`);


Add e to the debug message?

malept · 2018-03-12T22:28:38Z

src/util/get-electron-version.js

+  }
+
+  if (!result) {
+    d(`getElectronVersion failed to determine electron version: projectDir=${projectDir}, result=${result}`);


Electron version

malept · 2018-03-12T22:54:19Z

@jacobq I talked to @MarshallOfSound briefly and we decided to hold off on merging this after we've merged the v6 branch to master, due to the significant amount of refactoring in that branch.

jacobq · 2018-03-12T23:25:36Z

@malept OK, but I hope you'll keep this in mind / come back to it later. Thanks.

malept · 2018-03-12T23:30:01Z

We're not going to close this PR 😄 we're more likely going to rebase this on the merged master after v6 is merged.

malept · 2018-03-12T23:30:59Z

Also, I'm not immune to this, I have a PR that's being deferred for the same reason.

malept · 2018-04-10T03:42:36Z

I'm going to merge this into the 5.x branch and then forward-port the change into the 6.x branch.

Sometimes the official `electron-prebuilt-compile` package is not up to date with the latest `electron` release. In order to allow more flexibility in development and testing, we should allow developers to [specify a git URL](https://docs.npmjs.com/files/package.json#urls-as-dependencies) instead. This commit introduces a `getElectronVersion` utility to provide the exact version number when needed. For now it still relies on reading `package.json` but unifies the accesses through a single function.

Bumps [elliptic](https://github.com/indutny/elliptic) from 6.5.2 to 6.5.3. - [Release notes](https://github.com/indutny/elliptic/releases) - [Commits](indutny/elliptic@v6.5.2...v6.5.3) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

malept requested changes Mar 12, 2018

View reviewed changes

jacobq force-pushed the feat/allow-commitish-epc branch from 637bda6 to 1d701ad Compare March 12, 2018 22:03

malept changed the title ~~Feat/allow commitish epc~~ feat(generic): allow specifying electron-prebuilt-compile via URL Mar 12, 2018

malept requested changes Mar 12, 2018

View reviewed changes

jacobq mentioned this pull request Mar 13, 2018

Allow selective resolution of top-level dependencies yarnpkg/yarn#5505

Open

malept changed the base branch from master to 5.x April 10, 2018 03:42

jacobq added 3 commits April 9, 2018 20:46

feat(generic): allow specifying electron-prebuilt-compile via URL

7b33f31

refactor(generic): incorporate changes requested in PR

de1634b

malept force-pushed the feat/allow-commitish-epc branch from 598e330 to de1634b Compare April 10, 2018 03:46

malept approved these changes Apr 10, 2018

View reviewed changes

malept merged commit 489a25c into electron:5.x Apr 10, 2018

jacobq mentioned this pull request Apr 8, 2019

Please publish releases for 3.1.9, 4.2.0, and 5.0.1 electron-userland/electron-prebuilt-compile#77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

feat(generic): allow specifying electron-prebuilt-compile via URL #450

feat(generic): allow specifying electron-prebuilt-compile via URL #450

jacobq commented Mar 12, 2018 •

edited

Loading

malept left a comment •

edited

Loading

malept Mar 12, 2018

jacobq Mar 12, 2018

malept Mar 12, 2018

malept Mar 12, 2018

malept Mar 12, 2018

malept Mar 12, 2018

malept Mar 12, 2018

jacobq commented Mar 12, 2018

jacobq commented Mar 12, 2018

jacobq commented Mar 12, 2018

malept Mar 12, 2018

malept Mar 12, 2018

malept Mar 12, 2018

malept Mar 12, 2018

malept Mar 12, 2018

malept commented Mar 12, 2018

jacobq commented Mar 12, 2018

malept commented Mar 12, 2018

malept commented Mar 12, 2018

malept commented Apr 10, 2018

feat(generic): allow specifying electron-prebuilt-compile via URL #450

feat(generic): allow specifying electron-prebuilt-compile via URL #450

Conversation

jacobq commented Mar 12, 2018 • edited Loading

malept left a comment • 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

jacobq commented Mar 12, 2018

jacobq commented Mar 12, 2018

jacobq commented Mar 12, 2018

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

malept commented Mar 12, 2018

jacobq commented Mar 12, 2018

malept commented Mar 12, 2018

malept commented Mar 12, 2018

malept commented Apr 10, 2018

jacobq commented Mar 12, 2018 •

edited

Loading

malept left a comment •

edited

Loading