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

Don't merge, just a test: Electron to 11.3.0 #22011

Closed
wants to merge 1 commit into from

Conversation

mfonville
Copy link
Contributor

Is same npm major version, and no breaking changes according to electron docs.

Let's see if upgrade path would be easy, and getting back to supported electron versions (support for electron 9 has just been dropped when we finally got to it...)

Is same npm major version, and no breaking changes according to electron docs
@sadick254
Copy link
Contributor

sadick254 commented Mar 8, 2021

Hey @mfonville the CI is failing because of the native modules that Atom uses. They need to be updated in order for Atom to build successfully. We build native packages using nan, which means we have to update our native modules whenever we update electron. Different electron versions use different V8 header files.

Our plan was to migrate the native modules to use the N-API provided by Node. That way we would be shielded from V8 changes.

Let me know if there anything you need assistance with. I would be happy to help or point you to the right direction to get the electron upgrade going.

@mfonville
Copy link
Contributor Author

@sadick254 I don't have that much experience with Electron, and none with nan.
I wanted to test if it would be trivial or not to upgrade Electron to v11, because no major breakage is to be expected according to the Electron and npm docs.

If you could help me with migrating the native modules, that would be much appreciated. Just know that I am 100% clueless about it at the moment ;-)

@sadick254
Copy link
Contributor

@mfonville We can still be on the supported version of electron 10.4.0. Version 10.4.0 builds successfully but you will have to resolve an error on the text-editor-element.js. The error may require little effort to fix.

Screenshot 2021-03-10 at 19 59 13

Are you ok to try downgrading to 10.4.0?

@mfonville
Copy link
Contributor Author

mfonville commented Mar 10, 2021

@sadick254 I also got a successful build already with Electron v11 on my Linux machine that I think gave the same error when running. I will check it out once more and see if it is so.
Otherwise indeed going to v10 would be more easy. Could you still point me in the direction how to make sure I get native modules correctly? Is that just npm rebuild or does it need more magic?

I also noticed during building that we are using an outdated, no longer supported version of highlight.js. Maybe that needs attention from a developer too for an upgrade?

Update: I checked, and Electron v11 is indeed the exact same error. So probably no reason to stick to Electron 10 instead.

Copy link
Contributor

@aminya aminya left a comment

Choose a reason for hiding this comment

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

There are still many bugs in the Electron 9 upgrade. It is better to have a working and stable Atom before we rush into upgrading Electron again.

Here are some of the issues:
#22018
#22024
atom/tree-view#1371
nteract/hydrogen#2034

Also as mentioned in:
#21777 (comment)

app.allowRendererProcessReuse was set to false in Electron 9, so we can keep using non-context-aware native modules.

Pretty much all non N-api packages should be updated. If using NAN, we should make them context-aware like this: atom/node-keytar#182

, or they should be adjusted to use N-API.

This should be done before Electron 12 based on electron/electron#18397

❯ (electron) The default value of app.allowRendererProcessReuse is deprecated, it is currently "false".  It will change to be "true" in Electron 9.  For more information please check https://github.com/electron/electron/issues/18397
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\scrollbar-style\build\Release\scrollbar-style-observer.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\text-buffer\node_modules\superstring\build\Release\superstring.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\pathwatcher\build\Release\pathwatcher.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\git-utils\build\Release\git.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-json\build\Release\tree_sitter_json_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\oniguruma\build\Release\onig_scanner.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\Users\aminy\Documents\GitHub\JavaScript\@atom\spell-check\node_modules\spellchecker\build\Release\spellchecker.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\Users\aminy\.atom\packages\Hydrogen\node_modules\zeromq\build\Release\zmq.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-c\build\Release\tree_sitter_c_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-cpp\build\Release\tree_sitter_cpp_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-css\build\Release\tree_sitter_css_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-go\build\Release\tree_sitter_go_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-embedded-template\build\Release\tree_sitter_embedded_template_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-html\build\Release\tree_sitter_html_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-java-dev\build\Release\tree_sitter_java_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-javascript\build\Release\tree_sitter_javascript_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-jsdoc\build\Release\tree_sitter_jsdoc_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-regex\build\Release\tree_sitter_regex_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-python\build\Release\tree_sitter_python_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-ruby\build\Release\tree_sitter_ruby_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-rust\build\Release\tree_sitter_rust_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-bash\build\Release\tree_sitter_bash_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-typescript\build\Release\tree_sitter_tsx_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\tree-sitter-typescript\build\Release\tree_sitter_typescript_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:31924) Electron: Loading non-context-aware native module in renderer: '\\?\C:\atom\out\Atom Dev x64\resources\app.asar.unpacked\node_modules\@atom\nsfw\build\Release\nsfw.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.

@mfonville
Copy link
Contributor Author

mfonville commented Mar 11, 2021

@aminya Sorry for giving the impression wanting to rush an Electron 11 upgrade.
I just wanted to see where we are, and what would have to be done to work to such an upgrade. Also to get exactly these kind of deprecation notices and other kind of issues already in sight on time. So that they won't be too much of a surprise, which happened when lagging a lot behind Electron development.

So this is not meant at all at as "let's rush to new Electron" but more as "let's see what issues needs resolving before we can think of an upgrade". Because we are kind of "lucky" that at least no major breakage of Electron itself is expected from 9 to 11. The step to 12 will probably be much harder with API changes and a major npm bump.

From your comment I understand that getting N-API / context-aware support in all packages should be one of the priorities. Because we can expect many more Electron upgrades in the future.

@mfonville
Copy link
Contributor Author

@sadick254 (or any of the other devs) could you please have a look and help me out with my current draft?
https://github.com/mfonville/atom/tree/customElement

I am fixing up those registerElement calls with the new CustomElementRegistry as is the new "v1" method for web components (more info: https://developers.google.com/web/updates/2019/07/web-components-time-to-upgrade ) but I am now stuck at:

image

which refers to this line:
https://github.com/mfonville/atom/blob/customElement/src/styles-element.coffee#L34

Could you give it a look and please help me move forward. And maybe someone should have a look it if moving from v0 to v1 would even need a bigger overhaul than this quick patching.
In our current electron these methods are already deprecated, and from M80 they are dropped completely.

@mfonville mfonville marked this pull request as draft March 15, 2021 14:39
@aminya
Copy link
Contributor

aminya commented Mar 15, 2021

I am fixing up those registerElement calls with the new CustomElementRegistry as is the new "v1" method for web components (more info: developers.google.com/web/updates/2019/07/web-components-time-to-upgrade ) but I am now stuck at:

Having separate PRs to the "master" branch to attack separate issues can be easier to manage, test, verify, and review (compared to putting everything in one giant PR). That's why in Electron 9 PR, there are still bugs that are not fixed. For example, I don't know when exactly the context-menu actions stopped working because there were so many changes, it is very hard to find the root cause.

Regarding the question, here are two links to follow:

https://developer.mozilla.org/en-US/docs/Web/API/Document/registerElement
https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/define

@mfonville
Copy link
Contributor Author

@aminya I know, about those links, I am using them. But after updating the parts as per that new CustomElementRegistry I am now hitting that error from the StylesElement Coffeescript. Which doesn't directly touch the part I have changed, so some other mechanism has changed with it, and I lack the knowledge/overview to deal with it at the moment.

This code is also not filed as a PR yet, and would become a separate PR anyhow, since it even benefits the current Electron-9 branch and is something that needs work before we could even upgrade Electron.

If you please could look at the error I am dealing with, to put me in the right direction to fix it, that'd be much appreciated.

@mfonville
Copy link
Contributor Author

I am still stuck on this error. If anyone could please help me out, otherwise I'd have to abandon the effort to migrate to the customElements.

@sadick254
Copy link
Contributor

@mfonville I have opened another PR to upgrade electron to version 11.4.7. I think it will be better for us to collaborate on that PR #22687. I am going to close this for now.

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

3 participants