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

Electron Forge 7.0.0 and newer does not handle correctly older @electron-forge/maker-base and third-party makers. #3426

Open
3 tasks done
SpacingBat3 opened this issue Nov 25, 2023 · 2 comments

Comments

@SpacingBat3
Copy link

SpacingBat3 commented Nov 25, 2023

Pre-flight checklist

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

Electron Forge version

7.0.0 and newer

Electron version

(irrelevant, this is a Forge issue)

Operating system

(irrelevant, the issue is not associated with any platform-specific code in Forge)

Last known working Electron Forge version

6.x.y

Expected behavior

When there's an older and incompatible maker-base used by a maker, Forge should gracefully handle such case by like implementing a backwards compatibility logic for them or ignoring makers that use too old maker-base class and warning end-users about this.

Actual behavior

Electron Forge does quite a naive check for makers in order to verify if they are actually makers. This is currently problematic when extending Forge by own makers' implementation and causes stuff to happen like this: SpacingBat3/ReForged#12.

Steps to reproduce

  1. Install a maker depending on maker-base:^6.0.0 (i.e. anything older than 7.0.0).
  2. Install Forge 7.0.0 or newer.
  3. Configure it to use the maker.
  4. See an uncaught exception pointing to the use of .clone method (it is very likely undefined in older maker-base versions, or at least not a function) in this line (the commit in permalink is the exact commit that introduces this regression):
    makers.push(() => maker.clone());

Additional information

Related commit: 7370d6e
Related PR: #3363


This issue could also be associated with another API components, like publishers and plugins (I haven't checked how Forge handles detection of them).

@MarshallOfSound
Copy link
Member

When there's an older and incompatible maker-base used by a maker, Forge should gracefully handle such case by like implementing a backwards compatibility logic for them or ignoring makers that use too old maker-base class and warning end-users about this.

Our stance (although not sure how documented this is, cc doc wizard man @erickzhao) is that all versions of your forge dependencies should exactly match for optimum compatibility. In this case I think the correct thing to do is:

  • Third party folks need to publish new versions that depend on *-base@7.0.0
  • We could probably do a better job at warning folks about version mismatches like this, but tbh it's a hard problem to solve. We probably don't want to hard block folks from doing something that might cause problems. But we definitely want to surface it to them 🤔

@SpacingBat3
Copy link
Author

Our stance (...) is that all versions of your forge dependencies should exactly match for optimum compatibility. In this case I think the correct thing to do is:

  • Third party folks need to publish new versions that depend on *-base@7.0.0

  • We could probably do a better job at warning folks about version mismatches like this, but tbh it's a hard problem to solve. We probably don't want to hard block folks from doing something that might cause problems. But we definitely want to surface it to them 🤔

I've personally got a few ideas, I've been actually analysing the Forge's code for a bit of time around the checking and I think there's a few ways to enforce the use of compatible version within the makers used by Forge:

  1. One of the simplest ideas that doesn't require a modification of code would be a use of peerDependencies, in order to make NPM block installing a conflicting version of maker or Forge and to ensure there's no duplicate installations of maker-base. But I guess that would have to be also configured correctly on third-party makers' side to work and wouldn't change any behaviour if maker-base would be a just a regular dependency in one of the older makers (and this is basically what third-party makers are doing right now, especially those older ones). I still think this is one of the best solutions in order to entirely disallow combining Forge with makers based on different base that it is used in Forge (if you aim for that).

  2. Currently Forge also makes a use of __isElectronForgeMaker to detect if given class is actually a maker or not, I guess we could reuse this property (or create a new one if you're not OK with the name and possibly deprecate __isElectronForgeMaker) as a number that points of the scheme version (when comparing, the one could use the regular equality == to let JS convert booleans to numbers as a backwards compatibility mechanism). This could then be bumped on a breaking changes done to the class and in code used to just skip the makers that aren't extending the correct class. This also means that older versions of maker-base could be used with newer Forge or the opposite, as long as isn't considered a breaking change to the scheme.

  3. Forge could also do a simple check with instanceof operator or .constructor properties, as I believe Node will cache the class as long as both Forge and maker loads it from the same module. However, I honestly dunno what would happen if the file would be loaded twice (in cases when cache is modified), so I guess this isn't that much reliable method.

(...) although not sure how documented this is (...)

There isn't much of the info around how to depend on maker-base or the policy around the compatibility with the older bases, I suppose package.json only shows that all official components of Forge should be at the exactly same version. I honestly dunno how to depend on Forge in way end users can use the latest available base version within their build env yet to avoid API breakages in base causing incompatibilities between makers and base (I suppose this would be very likely to happen if Forge were to be rewritten again for any reason, or API were redesigned in some way).

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

No branches or pull requests

2 participants