-
Notifications
You must be signed in to change notification settings - Fork 562
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
Update Electron and associated dependencies #1771
Conversation
Tried to take a closer look at the Windows code-signing failure, which I'm running into locally.
===================
|
b151c3e
to
3d03790
Compare
bddeb88
to
750c971
Compare
Testing
Are you able to build this locally? |
Windows build Testing
|
Linux build Testing
|
Windows issues
|
This should be fixed in |
This updates the Electron dependencies to the latest. In doing so we need to have a minimum of Node 12 which is required by Electron 7. We also need to move the Windows build from AppVeyor to CircleCi.
7537f51
to
acd8ea6
Compare
Both checklists complete, no new bugs. I think it is ready to ship!!! |
import( | ||
/* | ||
webpackChunkName: 'source-importer', | ||
webpackPrefetch: true, |
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.
is there an explanation for why we're removing this? I didn't see any webpack
version changes or implications from the rest of the PR why this would need to go.
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.
From Slack:
init.ts:204 Unable to load preload script: ~/simplenote-electron/desktop/preload.js
I was unable to reproduce this error but I have seen similar ones before. It seems like this is probably fine to leave in but it's also the only invocation of it in the repository. The directive will preload the script during CPU idle time, meaning that imports start a hair earlier.
All that being said, while I think it's going to be better either leaving this in or taking it out in a separate PR with an explanation for the removal, it seems like such a small enough issue and already that it's an outlier that I don't think we need to block this PR to undo the change.
Testing
|
With further testing this appears related to tags more than content |
Fix
This updates the Electron dependencies to the latest. In doing so we need to
have a minimum of Node 12 which is required by Electron 7. We also need to move
the Windows build from AppVeyor to CircleCi.
Test
Test the Electron builds. This is a multiple point update so we should expect something to break.
Review
Only one developer is required to review these changes, but anyone can perform the review.
Release
RELEASE-NOTES.txt
was updated with: