Skip to content
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

Handle infer cases where electron-prebuilt-compile points to a URL/path instead of a version #975

Closed
3 tasks done
jacobq opened this issue Apr 15, 2019 · 4 comments
Closed
3 tasks done

Comments

@jacobq
Copy link

jacobq commented Apr 15, 2019

Preflight Checklist

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Problem Description

As discussed in #909 the requirement to specify electron-prebuilt-compile as a semver string in package.json makes it difficult/impossible to use one's own tarball instead (especially important as electron-prebuilt-compile appears to be dying). This was partially addressed by #932 but that code still attempts to parse the version string.

Proposed Solution

Wrap the call to new semver.Range(electronVersion) in a try ... catch and if it can't be parsed then attempt to resolve it like we normally do instead of blowing up. I have done this in my fork here:

master...jacobq:support-alternative-epc-version-strings

Alternatives Considered

When semver fails to parse the version string we could try searching it for a simple dot-delimited version number. For most npm tarball URLs this will produce the right version number, but it is merely by coincidence and won't necessarily work with git repository references, etc.

Additional Information

@jacobq jacobq added the enhancement Feature request label Apr 15, 2019
@malept malept added the wontfix ❌ Maintainers have decided not to pursue this issue label Apr 15, 2019
@malept
Copy link
Member

malept commented Apr 15, 2019

I appreciate the time you've taken to write this up, but I don't think this is the right solution.

For your specific use case, Electron Packager is not the right level at which to solve this problem. If you weren't using Electron Forge (v5), I would just tell you to programmatically set the electronVersion option instead of trying to change the infer code do something that a very small percentage of users will actually need.

Since you're using Electron Forge v5, I would say that there could be some code added to getElectronVersion to support scoped forks of Electron/electron-prebuilt-compile modules, but that is a topic best suited for the Forge issue tracker, not here.

@jacobq
Copy link
Author

jacobq commented Apr 15, 2019

Yes, once electron-forge v6 is released/stable this shouldn't be needed any more, and it's fine if you don't want to go this route (I can use the fork in the mean time).

Still, I don't understand why it isn't considered a bug that specifying a valid electron-prebuilt-compile dependency (URLs are allowed by npm) whose version is easily resolvable through your existing mechanism causes an exception to be thrown.

Another solution I should've mentioned would be to drop support for versions of electron-prebuilt-compile that aren't resolvable in the usual way.

@malept malept added bug 🐛 and removed enhancement Feature request wontfix ❌ Maintainers have decided not to pursue this issue labels Apr 15, 2019
@malept malept changed the title Allow specifying electron-prebuilt-compile via URL Handle infer cases where electron/electron-prebuilt-compile points to a URL/path instead of a version Apr 15, 2019
@malept
Copy link
Member

malept commented Apr 15, 2019

I was reading this in the context of a feature request. Looking at it again, yes, it's a bug that Electron Packager isn't ignoring the cases where electron or electron-prebuilt-compile refers to a URL or path instead of a version range.

@malept malept reopened this Apr 15, 2019
@malept malept changed the title Handle infer cases where electron/electron-prebuilt-compile points to a URL/path instead of a version Handle infer cases where electron-prebuilt-compile points to a URL/path instead of a version Apr 15, 2019
@malept malept self-assigned this Apr 16, 2019
@jacobq
Copy link
Author

jacobq commented Apr 16, 2019

I was reading this in the context of a feature request.

My fault: that is how I filed it. Thanks for fixing. I'll try using the v14-develop branch instead of my fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants