chore(ember): Update & cleanup Ember SDK #6003
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AbhiPrasad
approved these changes
Oct 20, 2022
packages/ember/addon/instance-initializers/sentry-performance.ts
Outdated
Show resolved
Hide resolved
mydea
force-pushed
the
fn/update-ember
branch
from
418ad50 to
59ec209
Compare
mydea
commented
Oct 21, 2022
| @@ -355,6 +354,26 @@ function _instrumentInitialLoad(config: EmberSentryConfig) { | |||
| performance.clearMeasures(measureName); | |||
| } | |||
|
|
|||
| function _hasPerformanceSupport() { | |||
There was a problem hiding this comment.
@AbhiPrasad I ended up extracting this out like this. How do you normally handle it when checking something where TS "thinks" it cannot be undefined, but it can on some browsers? It didn't feel quite right to just cast const performance = window.performance as { ... }, as then you also need to do e.g. performance.clearMarks!() everywhere etc, which is also a bit weird 😅
There was a problem hiding this comment.
We don’t do this very well, I would just best effort it tbh.
size-limit report 📦
|
mydea
changed the title
Where possible, import the module directly.
In newer versions, built-in components are Glimmer components, which are not (yet) instrumented. So these will not show up in transaction spans.
This is the same as the default config that is set anyhow, so no need to repeat this for test env.
1. When running "normal" tests, only run for pinned version of ember. 2. When running on CI, test pinned version, 4.0, release & embroider. 3. Run `yarn test:all` to test all CI versions locally
We use this custom component instead of the built-in `<LinkTo>`, as that is an ember component in older versions, and a glimmer component in newer versions. Since glimmer components are, as of now, not instrumented, this leads to different test results.
The new/current types use newer TS features that we currently do not support :(
This did not play nicely with E2E tests, so disabling this for now...
mydea
force-pushed
the
fn/update-ember
branch
from
e068505 to
e0923e4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates ember-cli (and all other dev dependencies) to the latest versions, updating to newer format of things etc. along the way.
While doing this, I also cleaned up some workflow-specific things. Notably:
lint/testlintnow also actually checks if all types are OK (this was not really checked before)testnow only runs against the pinned version of ember-source (as of now, 4.8.0). Previously, we had some code to run ember-try against the current release, but allowed it to fail (probably to avoid noise). IMHO it makes more sense to simply only run against the known stuff, and then run the full compatibility checks on CI only.Compatibility
Speaking of compatibility, I have aligned the actually tested versions with the stated compatibility from the readme (ember 4+). Previously, we tested against 3.24 (and also the pinned version was 3.x). Note that realistically, nothing should actually change in support etc. with this PR.
Also, instead of running against beta on CI (but allowing to fail), I updated CI to instead only run against:
This should give a good coverage and safety that nothing important is breaking. Things like classic etc. are not that relevant, and it is highly unlikely (bordering on impossible) to have any impact on sentry code.
Tests
I made some minor adjustments to tests / the dummy app. Mainly, in newer versions the built-in components (
<LinkTo>etc) are glimmer components instead of ember components, which are, as of now, not instrumented. Leading to failing tests.Updates to actual package code
Some very minor adjustments are included here (necessitated by other updates), which should all be fully backwards compatible:
@ember/instrumentation) instead of usingEmber.xember-cli-typescriptdependency from 4.x to 5.x. The only breaking change there has been dropping support for older node versions, which we do not support here anyhow.A note on updating types
Please note that sadly, we cannot update the
@types/emberdependencies at all, as the newer types rely on TS features that we currently do not support, leading to failing builds. So we need to keep them as-is for now.