-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
Feat: v3 Refactor #361
Feat: v3 Refactor #361
Conversation
2b03836
to
95b71ae
Compare
e2587e5
to
e8d20de
Compare
ea682b8
to
e09800e
Compare
15d80ac
to
464ea56
Compare
464ea56
to
0480316
Compare
@timfish can we bring this to a resolution and maybe ship a beta release so users can start trying it out? |
Fixes #249 |
I think this might be ready for a beta! |
Can we create a new GH issue that covers a brief migration guide for the beta? We can then start iterating on that. We need to update changelog as well? |
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.
We can do the changelog in another commit or include it here - up to you. Thanks for the migration guide.
I think we good to
I've still got a little bit left to add to the migration guide. I'll finish it off and add a link to the changelog in this PR. |
Other than updating the changelog, these final changes change all integration constructor options from arguments into options objects. This improves readability and allows for additions and modifications later without introducing breaking changes. For example: new ElectronEvents(false);
// becomes
new ElectronEvents({ unresponsive: false }) |
"rootDir": "src", | ||
"target": "es5" | ||
"target": "es6" |
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.
Last minute double check - we good to change the target from es5
to es6
?
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.
I suppose I could be relying on the e2e tests too much here! They pass with Electron v2 so assumed everything is ok but we don't actually test all SDK features in the e2e tests yet 🤔
These changes make the minimum supported Electron version v2 which was node v8.9.3 and Chrome v61.
Apparently node 6.4.0 included almost all of ES2015 apart from proper tail call optimisation: https://node.green/
Chrome has supported the same since v51:
https://caniuse.com/es6
A quick search suggests v8 still doesn't support tail call optimisation so ES6 was essentially fully supported at these points for all intents and purposes 🤷♂️
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.
This completely passed me by but v8 decided not to ship tail call optimisation so never technically supported full ES6:
https://v8.dev/blog/modern-javascript?m=1#proper-tail-calls
This means "ES6" has been supported since Electron v1.3.7 which means we could have targeted it before when the minimum Electron version was v1.6.
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.
Ok gotcha. Do we need to write down the minimum version for electron somewhere then?
Also we can probably catch this quickly on a beta release if something goes wrong, so fine to bump this here.
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.
Minimum Electron version has been bumped to v2 in the readme
This PR refactors the Electron SDK into thin layers on top of the browser and node SDKs.
Electron functionality is added via optional integrations.
This PR:
@sentry/electron/main
,@sentry/electron/preload
and@sentry/electron/renderer
workpackage.json
exports
key so modern bundlers can find ESM output via sub-module importsMainProcessSession
) tracks the lifetime of the main processBrowserTracing
and transactionsstore
endpoint to test server for testing sending from the browser SDKOther Considerations...
Closes:
#360, #356, #355, #352, #351, #268, #254, #91