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

fix: require .node files directly to detect incompatible native modules #21927

Merged
merged 2 commits into from
Mar 13, 2021

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Feb 4, 2021

Description of the change

fix: require .node files directly to detect incompatible native modules

  • This fixes the incompatible native module detection for the packages that require their .node files lazily
  • Speeds up the performance of detection by directly require .node files instead of requiring the package

Fixed Issues

This bug is one of the reasons that all the terminal packages break whenever an Electron upgrade happens or when you don't use apm rebuild

An example of the packages that get fixed by this fix:
All the terminal packages that use node-pty-prebuilt-multiarch. Among them:

There are many other issues. I need time to list all of them

Verification

The CI passes.

A test you can do:

  • Download x-terminal package as an example
  • Run npm rebuild in its directory under .atom/packages/x-terminal
  • Now run atom. You will see that it appears in the list of incompatible packages. Before this PR, the package just used to break without appearing in the rebuild list.

image

src/package.js Outdated Show resolved Hide resolved
@aminya aminya force-pushed the incompatible-native-detection branch 4 times, most recently from de30f5e to ec3f95e Compare February 4, 2021 06:33
@aminya aminya force-pushed the incompatible-native-detection branch from ec3f95e to afaceb3 Compare March 6, 2021 06:20
- This fixes the incompatible native module detection for the packages 
that require their .node files lazily
- Speeds up the performance of detection by directly require .node files 
instead of requiring the package
@aminya aminya force-pushed the incompatible-native-detection branch from afaceb3 to ad2aaa8 Compare March 6, 2021 06:56
@aminya
Copy link
Contributor Author

aminya commented Mar 6, 2021

@sadick254 @darangi This addresses one of the biggest headaches when Atom ships a new Electron. Traditionally, people had to manually go and rebuild the .node files for the packages that load native modules dynamically. I strongly recommend including this fix in the same version that the Electron upgrade is shipped to prevent this issue from happening again.

@aminya aminya mentioned this pull request Mar 6, 2021
58 tasks
@the-j0k3r
Copy link

The support burden of each electron upgrade breaking current installs would be greatly alleviated by this PR, so thx at @aminya for these efforts.

@darangi
Copy link
Contributor

darangi commented Mar 10, 2021

Thanks for the effort 🎉 @aminya

@aminya
Copy link
Contributor Author

aminya commented Mar 11, 2021

You're welcome!

@aminya aminya mentioned this pull request Mar 11, 2021
1 task
@darangi darangi merged commit 7574784 into atom:master Mar 13, 2021
@the-j0k3r
Copy link

Congrats on merge, if this is backported into 1.56beta we may have some joy out of this sooner rater than later.

@the-j0k3r
Copy link

So apparently atom 1.56.0 doesn't include this fix see https://github.com/bus-stop/terminus/issues/219 could it be backported to next 1.56 release?

Copy link

@Clcatalan11 Clcatalan11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

White

@michakinchen1988
Copy link

T

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.

None yet

5 participants