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

node V12 test is really needed? #2660

Closed
eouia opened this issue Sep 10, 2021 · 4 comments
Closed

node V12 test is really needed? #2660

eouia opened this issue Sep 10, 2021 · 4 comments

Comments

@eouia
Copy link
Contributor

eouia commented Sep 10, 2021

Node V12 is not Active LTS. It is on the Maintenance LTS stage.

Current dependency Electron 13.2.3 is based on Node v14.17, Chromium 91, V8 9.1
And upcoming Electron 14.0.0 will be based on Node v14.17, Chromium 93, V8 9.3

So the node V12 test is obsoleted at this moment, I think.

@eouia
Copy link
Contributor Author

eouia commented Sep 10, 2021

I tried a new JS feature string.replaceAll() in my code but it fails the node v12/ node v14 tests because it was introduced since node v15.
But it is implemented in the browser already(since Chromium 85, Edge 85, Firefox77, Opera 71, Safari 13.1), most parts of MM's code would be running on the browser. So I cannot catch up with the meaning of these server-sided browser emulated tests.

@khassel
Copy link
Collaborator

khassel commented Sep 10, 2021

So the node V12 test is obsoleted at this moment, I think.

from my side the v12 tests could be removed, I never saw a different result between v12 and v14 as long as I look at them. End of life of v12 is 2022-04-30.

So I cannot catch up with the meaning of these server-sided browser emulated tests.

there is already #2408 but I found no time so far to start with this.

@rejas
Copy link
Collaborator

rejas commented Sep 10, 2021

Thanks for your input and I understand your reasoning , alas:

  • node12 being in Maintainance mode doesnt mean its not used anymore. It still gets security updates
  • Electron might use node14 internally, but people can (and probably do) run their MM just in server mode and point any brwoser to it.

As for your usecase: Would be easy to polyfill the replaceAll and add it to the code with a little note to remove it once node12 support is removed

@eouia
Copy link
Contributor Author

eouia commented Sep 10, 2021

Yes, I can make a polyfill for .replaceAll(). And I'll do it soon.

BTW, for node_helper or other server-sided parts, v12 test has the worth.

But to test browser(or electron)-sided parts, I still think that v12 test is meaningless.

Of course, there could be cases of really old browsers like Midori or IE, but MM is supporting them still? (I'm not sure.) Especially.replaceAll() is already implemented in common modern browsers more than 1 year ago. Chromium 85 was released 2020-08-25, Firefox 77 was released 2020-06-01. The only node lately accepts this feature. The fact of what feature is valid in the real browsers cannot pass the server-based mimic test looks somewhat irony to me. ;)

Anyway, I understand your points. This issue could be closed.

@eouia eouia closed this as completed Sep 10, 2021
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

3 participants