-
-
Notifications
You must be signed in to change notification settings - Fork 168
[DRAFT] Update to Electron 30 #1367
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
base: master
Are you sure you want to change the base?
Conversation
Abandoning support for the legacy Tree-sitter system is a necessary step toward upgrading past Electron 12.2.3.
…and switch all internal usages to `@electron/remote`. This commit also includes the entire bundle of changes to the `tree-view` package — the decaffeination of the `tree-view` specs made it very hard to isolate this change atomically, so I'm reluctantly deciding to bring the whole thing along. `tree-view` has been updated to fix a few bugs with Electron 30 (moving items to the trash, revealing items in the file explorer, etc.) and the specs have been fully decaffeinated (not just machine-decaffeinated).
…and point our dependencies to the NPM-published versions under the `@pulsar-edit` namespace that are compatible with Electron 30.0.9.
This vastly reduces the number of random GitHub commits we're pointing to in our `dependencies` and helps the `resolutions` section be as streamlined as it can be.
Also includes `@electron/remote` and `@electron/rebuild`.
All internal references to paths are also changed in this commit — e.g., `require('foo')` becomes `require('@pulsar-edit/foo')`.
…related to changes in Chromium sometime between Electron 12 and Electron 30. For instance, text bounding-box logic changed at some point to require that our four “sentinel” characters be isolated from one another — otherwise character height/width measurements are inaccurate. Also fixed some issues related to device pixel ratios that resulted in spec failures.
…in favor of a custom solution that uses the `typescript` package directly. `typescript-simple` is no longer updated and is arguably unnecessary anyway; it wasn't hard to replicate its behavior through direct consumption of `typescript`. This preserves our ability to transpile TypeScript just like we can transpile CoffeeScript or `@babel`-ified JS. This is not the happy path of TypeScript transpilation (do it yourself as a build step!) but it's behavior that exists, so we should ensure it continues to work.
…to replace the deprecated `scrollbar-style` module. macOS offers two different “styles” of scrollbar — traditional scrollbars and “overlay” scrollbars that do not use a dedicated scrollbar track. Strangely, we can detect when the user switches from one to the other without any external dependencies… but to find out imperatively which one is currently active, we need a very light module with native bindings. This commit adopts the new approach for detecting which macOS scrollbar style is enabled. Compared to the _status quo ante_, this should carry no behavior change on either macOS or other platforms.
…that probably should have gone in the earlier commit.
…via Electron's standard `crashReporter` interface. Crash reports are enabled on Windows and Linux by default (macOS’s own crash reports via `Console.app` are typically more helpful, but go away if crash reporting is enabled). But you can opt into crash reporting explicitly on all platforms via the `--crashdump` command-line switch.
…and disable a Blink experimental feature that was causing trouble with package compatibility (because it added an `on` method to all DOM nodes!).
…in case we want to maintain a `next` (or `nightly` or `preview`) release channel on an ongoing basis.
…so that the specs don't run out of memory. Also be more diligent about cleaning up parsers that we created for a given buffer when that buffer is destroyed.
…and other changes related to release-channel agnosticism. (These should've been included in an earlier commit.)
…and abandon `atom-pathspec` in favor of an internal implementation. (We are pinning to a specific commit of `node-spellchecker`. This one escaped my notice during the context-aware-native-modules sweep — probably because it's in a package! We should revisit this and publish it to NPM instead, but this is likely good enough for now.)
…when the browser can't give us an element under a given X/Y coordinate.
…to include Wayland-related keyboard dependencies. (This probably should be done for the Cirrus builds as well!)
…to include Wayland-related keyboard dependencies. (Untested, but copied directly from the GHA workflow files.)
…now that we're no longer affected by the awful `libuv` bug that was present in Electron 12.
…to support a couple of command-line switches and to build correctly for Electron 30.
…to work around `tree-view` issues related to trashing and revealing items in `electron.shell`. `atom.trashItem` and `atom.showItemInFolder` are drop-in replacements for `electron.shell.trashItem` and `electron.shell.showItemInFolder`. TODO: These might not be necessary; try the `@electron/remote` versions instead.
|
Some low-hanging fruit in the CI failures; I'll see if I can address them tonight. |
confused-Techie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a quick skim and noticed some references still to Pulsar-Next.
Since you said part of this PR is no longer defining itself as Pulsar-Next it seemed smart that we remove it wherever we find it. So small potatoes, but it's what I was able to quickly review.
Otherwise installing the binary and will begin testing on Windows 10 & 11
|
I'll see about taking a look at this later today. Apparently the nix shell shenanigans that I used to test Pulsar Next originally is on my desktop and not my laptop that I'm on currently For anyone unaware, built artifact downloads are linked inside the |
…since they weren't even being used. The equivalent functions from the `shell` module of `@electron/remote` have been performing the same task in PulsarNext for a while now.
|
I'm running this as my daily for now for testing on both Win11 and Win10 and haven't noticed anything right off the bat. What I will mention is there is an unfortunate size increase in the, specifically, "Pulsar Setup.exe" windows installer with:
|
Also a last tip, you must be logged into GitHub to be able to see the download icon on the mentioned page. If you are not logged in, you'll see the zip files and hashes but no way to download |
2d25c12 to
e35c753
Compare
|
I added a checklist of platforms against which to do sanity checks. I'm able to check the first three on the list and have done the first two. If you're able to take one of the other platforms, please do, and report back. By “sanity check” I just mean: download the binary from CI, ensure it runs correctly, and do some basic stuff to test syntax highlighting, file saving, and maybe package installation. |
|
Another thing that needs fixing: Rebuilding a package seems like it would work properly within the editor (using the So that definitely needs to be fixed in Separate from that: I had trouble with upgrading |
|
This is complicated. I've been trying to find a package that exemplifies the following characteristics:
This is actually quite rare! Here's why:
I wasn't aware of the ABI stability feature of N-API — but, sure enough, it's in the docs:
When I first upgraded from Pulsar (E12) to Pulsar (E30), I had the But (a) So I'm mentioning it here just to keep it on folks’ radar! |
|
Might have something to do with the precompile cache? |
|
Tested on Windows 11 (x64), looking good so far! Launched just fine. Syntax highlighting and editing (with reasonable auto-indent behavior) worked fine in some JSON, YAML, MD files in a very quick test. Saving those files and having the editor respond to edits made in Notepad with Pulsar still open also worked fine. Installing Hope to test Linux x64 soonish. |
|
Good results testing the basics on Fedora Workstation 43 (x64)! Ditto the above Windows 11 testing report, basically! Substitute That said, I did try installing Though I believe we are using tree sitter carp 0 3 0 - install failure log.txt So, seamless native package support is not necessarily there yet for at least some of the existing packages, at least as of my writing this. Seems the package testing effort remains worthwhile. |
I made commit 499a100 on another branch; It bumps ppm to include pulsar-edit/ppm#155. I can push it to this PR's branch or open a separate PR if you'd like. |
Nah, go ahead and add it to this one! |
That's the same error I got last year as catalogued in this issue. I didn't even remember that that happened, but I found the issue upon searching the |
Do not accidentally click any green buttons. We want at least three people on the core team to sign off on this before we land it.
This updates
masterto the contents ofupdated-latest-electron, more or less — except for the parts that should not land onmaster(PulsarNext-specific upload tokens, CI job differences related to release channel, etc.). The commit history in this PR is, uh… artificial, but it saves us from a bunch of low-content commits that do nothing but bump individual dependencies over a period of 2+ years. This is the better way.This PR will kick off CI tests and should build new binaries in CI; these binaries should be tested as widely as possible. I've got macOS/Apple Silicon covered (and could do macOS/Intel in a pinch, along with Windows 10), but I would love it if other folks stepped up for Linux and Windows just to make sure this works as we expect and nothing explodes. These binaries do not call themselves PulsarNext and do not use a separate home directory, so testing these binaries should also simulate the average user's experience after upgrading to this version; any packages they've got installed with native module dependencies will need to be rebuild before they work, and that could cause headaches, so I'm hoping we can get out ahead of that stuff and have a useful blog post ready when the release drops so that users can be told what to expect.
Things I'd like to settle before landing this:
atom.trashItemandatom.showItemInFolder, but I think these might be unnecessary; we should tryrequire('@electron/remote').shell.trashItemandrequire('@electron/remote').shell.showItemInFolderand prefer those if they work.updated-latest-electronbranch to install any of the Wayland-related build dependencies, so I'm not sure if any such binaries built by Cirrus actually support Wayland. I didn't catch this oversight until now. I updated.cirrus.ymlto include these dependencies, but if we want to confirm that this results in ARM Linux builds that support Wayland, we'll have to manually trigger a Cirrus job from this PR branch.ppm rebuildas described below. EDIT: Before this PR lands, this other PR fromppmmust land and we must update theppmsubmodule reference in this PR.But none of this should preclude review of this PR. Pick it apart! Go nuts! Consume the CI artifacts! We're really close to the finish line here!
Platform sanity checks