fix: require .node files directly to detect incompatible native modules by aminya · Pull Request #21927 · atom/atom · GitHub
Skip to content
This repository was 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
darangi merged 2 commits into
atom:masterfrom
atom-community:incompatible-native-detection
Mar 13, 2021
Merged

darangi merged 2 commits into
atom:masterfrom
atom-community:incompatible-native-detection

Conversation

@aminya

@aminya aminya commented Feb 4, 2021 •

Copy link
Copy Markdown
Contributor

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

Comment thread src/package.js Outdated
@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

aminya commented Mar 6, 2021 •

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

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

darangi commented Mar 10, 2021

Copy link
Copy Markdown
Contributor

Thanks for the effort 🎉 @aminya

@aminya

aminya commented Mar 11, 2021

Copy link
Copy Markdown
Contributor Author

You're welcome!

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

Copy link
Copy Markdown

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
Copy Markdown

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?

@Clcatalan11 Clcatalan11 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

White

@michakinchen1988

Copy link
Copy Markdown

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.

5 participants