Skip to content

chore(ember): Update & cleanup Ember SDK #6003

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

Merged
merged 14 commits into from
Oct 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -386,11 +386,11 @@ jobs:
name: Test @sentry/ember
needs: [job_get_metadata, job_build]
continue-on-error: true
timeout-minutes: 30
timeout-minutes: 10
runs-on: ubuntu-latest
strategy:
matrix:
scenario: [ember-release, ember-beta, ember-classic, ember-lts-3.24]
scenario: [ember-release, embroider-optimized, ember-4.0]
steps:
- name: Check out current commit (${{ needs.job_get_metadata.outputs.commit_label }})
uses: actions/checkout@v3
Expand All @@ -404,11 +404,8 @@ jobs:
- name: Set up Node
uses: actions/setup-node@v3
with:
# The only danger with Ember in terms of Node versions is that the build tool, ember-cli, won't work if Node
# is too old. Since Oct 2019, Node 10 has been the oldest version supported by ember-cli, so test against
# that. If it passes, newer versions of Node should also be fine. This saves us from having to run the Ember
# tests in our Node matrix above.
node-version: '10'
# We support node 14+. If that works, we can safely assume that newer versions will also work.
node-version: '14'
- name: Check dependency cache
uses: actions/cache@v3
with:
Expand Down
8 changes: 7 additions & 1 deletion packages/ember/.ember-cli
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,11 @@

Setting `disableAnalytics` to true will prevent any data from being sent.
*/
"disableAnalytics": false
"disableAnalytics": false,

/**
Setting `isTypeScriptProject` to true will force the blueprint generators to generate TypeScript
rather than JavaScript by default, when a TypeScript version of a given blueprint is available.
*/
"isTypeScriptProject": true
}
5 changes: 5 additions & 0 deletions packages/ember/.eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,13 @@
# misc
/coverage/
!.*
.*/
.eslintcache

# ember-try
/.node_modules.ember-try/
/bower.json.ember-try
/npm-shrinkwrap.json.ember-try
/package.json.ember-try
/package-lock.json.ember-try
/yarn.lock.ember-try
23 changes: 14 additions & 9 deletions packages/ember/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,16 @@ module.exports = {
// node files
{
files: [
'.eslintrc.js',
'.template-lintrc.js',
'ember-cli-build.js',
'index.js',
'testem.js',
'blueprints/*/index.js',
'config/**/*.js',
'tests/dummy/config/**/*.js',
'./.eslintrc.js',
'./.prettierrc.js',
'./.template-lintrc.js',
'./ember-cli-build.js',
'./index.js',
'./testem.js',
'./blueprints/*/index.js',
'./config/**/*.js',
'./tests/dummy/config/**/*.js',
],
excludedFiles: ['addon/**', 'addon-test-support/**', 'app/**', 'tests/dummy/app/**'],
parserOptions: {
sourceType: 'script',
},
Expand All @@ -47,5 +47,10 @@ module.exports = {
plugins: ['node'],
extends: ['plugin:node/recommended'],
},
{
// test files
files: ['tests/**/*-test.{js,ts}'],
extends: ['plugin:qunit/recommended'],
},
],
};
6 changes: 6 additions & 0 deletions packages/ember/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,10 @@
# ember-try
/.node_modules.ember-try/
/bower.json.ember-try
/npm-shrinkwrap.json.ember-try
/package.json.ember-try
/package-lock.json.ember-try
/yarn.lock.ember-try

# broccoli-debug
/DEBUG/
7 changes: 7 additions & 0 deletions packages/ember/.npmignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
/.eslintignore
/.eslintrc.js
/.git/
/.github/
/.gitignore
/.prettierignore
/.prettierrc.js
/.template-lintrc.js
/.travis.yml
/.watchmanconfig
Expand All @@ -26,11 +29,15 @@
/ember-cli-build.js
/testem.js
/tests/
/yarn-error.log
/yarn.lock
/.npmignore
.gitkeep

# ember-try
/.node_modules.ember-try/
/bower.json.ember-try
/npm-shrinkwrap.json.ember-try
/package.json.ember-try
/package-lock.json.ember-try
/yarn.lock.ember-try
2 changes: 1 addition & 1 deletion packages/ember/.template-lintrc.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
'use strict';

module.exports = {
extends: 'octane',
extends: 'recommended',
};
3 changes: 2 additions & 1 deletion packages/ember/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ ENV['@sentry/ember'] = {

### Supported Versions

`@sentry/ember` currently supports Ember **4.0+** for error monitoring.
* **Ember.js**: v4.0 or above
* **Node**: v14 or above

### Previous Integration

Expand Down
6 changes: 0 additions & 6 deletions packages/ember/addon/ember.d.ts

This file was deleted.

35 changes: 28 additions & 7 deletions packages/ember/addon/instance-initializers/sentry-performance.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import ApplicationInstance from '@ember/application/instance';
import Ember from 'ember';
import { run, _backburner, scheduleOnce } from '@ember/runloop';
import { subscribe } from '@ember/instrumentation';
import * as Sentry from '@sentry/browser';
import { ExtendedBackburner } from '@sentry/ember/runloop';
import { Span, Transaction, Integration } from '@sentry/types';
Expand All @@ -9,6 +9,7 @@ import { getActiveTransaction } from '..';
import { browserPerformanceTimeOrigin, GLOBAL_OBJ, timestampWithMs } from '@sentry/utils';
import { macroCondition, isTesting, getOwnConfig } from '@embroider/macros';
import { EmberSentryConfig, GlobalConfig, OwnConfig } from '../types';
import RouterService from '@ember/routing/router-service';

function getSentryConfig() {
const _global = GLOBAL_OBJ as typeof GLOBAL_OBJ & GlobalConfig;
Expand Down Expand Up @@ -279,7 +280,6 @@ function _instrumentComponents(config: EmberSentryConfig) {
const beforeEntries = {} as RenderEntries;
const beforeComponentDefinitionEntries = {} as RenderEntries;

const subscribe = Ember.subscribe;
function _subscribeToRenderEvents() {
subscribe('render.component', {
before(_name: string, _timestamp: number, payload: Payload) {
Expand Down Expand Up @@ -309,22 +309,21 @@ function _instrumentInitialLoad(config: EmberSentryConfig) {
const startName = '@sentry/ember:initial-load-start';
const endName = '@sentry/ember:initial-load-end';

const { performance } = window;
const HAS_PERFORMANCE = performance && performance.clearMarks && performance.clearMeasures;
let { HAS_PERFORMANCE, HAS_PERFORMANCE_TIMING } = _hasPerformanceSupport();

if (!HAS_PERFORMANCE) {
return;
}

const { performance } = window;

if (config.disableInitialLoadInstrumentation) {
performance.clearMarks(startName);
performance.clearMarks(endName);
return;
}

// Split performance check in two so clearMarks still happens even if timeOrigin isn't available.
const HAS_PERFORMANCE_TIMING =
performance.measure && performance.getEntriesByName && browserPerformanceTimeOrigin !== undefined;
if (!HAS_PERFORMANCE_TIMING) {
return;
}
Expand Down Expand Up @@ -355,6 +354,26 @@ function _instrumentInitialLoad(config: EmberSentryConfig) {
performance.clearMeasures(measureName);
}

function _hasPerformanceSupport() {
Copy link
Member Author

Choose a reason for hiding this comment

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

@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 😅

Copy link
Member

Choose a reason for hiding this comment

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

We don’t do this very well, I would just best effort it tbh.

// TS says that all of these methods are always available, but some of them may not be supported in older browsers
// So we "pretend" they are all optional in order to be able to check this properly without TS complaining
const _performance = window.performance as {
clearMarks?: Performance['clearMarks'];
clearMeasures?: Performance['clearMeasures'];
measure?: Performance['measure'];
getEntriesByName?: Performance['getEntriesByName'];
};
const HAS_PERFORMANCE = Boolean(_performance && _performance.clearMarks && _performance.clearMeasures);
const HAS_PERFORMANCE_TIMING = Boolean(
_performance.measure && _performance.getEntriesByName && browserPerformanceTimeOrigin !== undefined,
);

return {
HAS_PERFORMANCE,
HAS_PERFORMANCE_TIMING,
};
}

export async function instrumentForPerformance(appInstance: ApplicationInstance) {
const config = getSentryConfig();
const sentryConfig = config.sentry;
Expand All @@ -372,7 +391,9 @@ export async function instrumentForPerformance(appInstance: ApplicationInstance)
new tracing.Integrations.BrowserTracing({
routingInstrumentation: (customStartTransaction, startTransactionOnPageLoad) => {
const routerMain = appInstance.lookup('router:main');
let routerService = appInstance.lookup('service:router');
let routerService = appInstance.lookup('service:router') as
| RouterService & { externalRouter?: RouterService; _hasMountedSentryPerformanceRouting?: boolean };

if (routerService.externalRouter) {
// Using ember-engines-router-service in an engine.
routerService = routerService.externalRouter;
Expand Down
67 changes: 29 additions & 38 deletions packages/ember/config/ember-try.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';

const getChannelURL = require('ember-source-channel-url');
const { embroiderSafe } = require('@embroider/test-setup');
const { embroiderOptimized } = require('@embroider/test-setup');

/**
* Pick which versions of ember against which to test based on whether the tests are running locally, as part of a PR,
Expand All @@ -19,50 +19,41 @@ module.exports = async function () {
'ember-source': await getChannelURL('release'),
},
},
allowedToFail: true,
},
];

// in CI we add a few more tests - LTS and embroider (which is an ember compiler)
if (process.env.GITHUB_ACTIONS) {
scenarios = scenarios.concat([
{
name: 'ember-lts-3.24',
npm: {
devDependencies: {
'ember-source': '~3.24.0',
},
{
name: 'ember-4.0',
npm: {
devDependencies: {
'ember-source': '~4.0.1',
},
},
embroiderSafe(),
]);
scenarios = scenarios.concat([
{
name: 'ember-beta',
npm: {
devDependencies: {
'ember-source': await getChannelURL('beta'),
},
},
embroiderOptimized(),
{
name: 'ember-beta',
npm: {
devDependencies: {
'ember-source': await getChannelURL('beta'),
},
allowedToFail: true,
},
{
name: 'ember-classic',
env: {
EMBER_OPTIONAL_FEATURES: JSON.stringify({
'application-template-wrapper': true,
'default-async-observers': false,
'template-only-glimmer-components': false,
}),
},
npm: {
ember: {
edition: 'classic',
},
allowedToFail: true,
},
{
name: 'ember-classic',
env: {
EMBER_OPTIONAL_FEATURES: JSON.stringify({
'application-template-wrapper': true,
'default-async-observers': false,
'template-only-glimmer-components': false,
}),
},
npm: {
ember: {
edition: 'classic',
},
},
]);
}
},
];

return {
useYarn: true,
Expand Down
11 changes: 9 additions & 2 deletions packages/ember/ember-cli-build.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const EmberAddon = require('ember-cli/lib/broccoli/ember-addon');

module.exports = function (defaults) {
let app = new EmberAddon(defaults, {
const app = new EmberAddon(defaults, {
// Add options here
});

Expand All @@ -13,6 +13,13 @@ module.exports = function (defaults) {
This build file does *not* influence how the addon or the app using it
behave. You most likely want to be modifying `./index.js` or app's build file
*/

const { maybeEmbroider } = require('@embroider/test-setup');
return maybeEmbroider(app);
return maybeEmbroider(app, {
skipBabel: [
{
package: 'qunit',
},
],
});
};
Loading