diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index faa7c0d9ca8..c6fd88fabd8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,6 +102,29 @@ jobs: ALL_DEPRECATIONS_ENABLED: true run: pnpm test + deprecations-broken-test: + name: Debug and Prebuilt (All Tests by Package + Canary Features) with Deprecations as Errors + runs-on: ubuntu-latest + needs: [ basic-test, lint, types ] + steps: + - uses: actions/checkout@v3 + - uses: ./.github/actions/setup + - name: build + env: + DISABLE_SOURCE_MAPS: true + BROCCOLI_ENV: production + run: pnpm ember build + - name: Upload build + uses: actions/upload-artifact@v3 + with: + name: dist + path: dist + - name: test + env: + TEST_SUITE: each-package + OVERRIDE_DEPRECATION_VERSION: '15.0.0' # Throws on all deprecations with an until before or equal to this version + run: pnpm test + browserstack-test: name: Browserstack Tests (Safari, Edge) runs-on: ubuntu-latest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cd79e2912fe..66f657c5463 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -309,13 +309,13 @@ can be constructed in advance of the deprecation being added to the [deprecation app](https://github.com/ember-learn/deprecation-app) by following this format: `https://deprecations.emberjs.com/deprecations/{{id}}`. -`deprecate` should then be called using the entry from the `DEPRECATIONS` object. +`deprecateUntil` (internal to Ember) should then be called using the entry from the `DEPRECATIONS` object. ```ts -import { DEPRECATIONS } from '@ember/-internals/deprecations'; +import { DEPRECATIONS, deprecateUntil } from '@ember/-internals/deprecations'; //... -deprecate(message, DEPRECATIONS.MY_DEPRECATION.test, DEPRECATIONS.MY_DEPRECATION.options); +deprecateUntil(message, DEPRECATIONS.MY_DEPRECATION); ``` diff --git a/bin/run-tests.js b/bin/run-tests.js index 911efb31e96..ac77c81631f 100755 --- a/bin/run-tests.js +++ b/bin/run-tests.js @@ -43,6 +43,10 @@ function run(queryString) { queryString = `${queryString}&alldeprecationsenabled=${process.env.ALL_DEPRECATIONS_ENABLED}`; } + if (process.env.OVERRIDE_DEPRECATION_VERSION) { + queryString = `${queryString}&overridedeprecationversion=${process.env.OVERRIDE_DEPRECATION_VERSION}`; + } + let url = 'http://localhost:' + PORT + '/tests/?' + queryString; return runInBrowser(url, 3); } diff --git a/packages/@ember/-internals/deprecations/index.ts b/packages/@ember/-internals/deprecations/index.ts index 129dd745a43..33dc91a155a 100644 --- a/packages/@ember/-internals/deprecations/index.ts +++ b/packages/@ember/-internals/deprecations/index.ts @@ -1,15 +1,37 @@ import type { DeprecationOptions } from '@ember/debug/lib/deprecate'; import { ENV } from '@ember/-internals/environment'; +import { VERSION } from '@ember/version'; +import { deprecate, assert } from '@ember/debug'; function isEnabled(options: DeprecationOptions) { return Object.hasOwnProperty.call(options.since, 'enabled') || ENV._ALL_DEPRECATIONS_ENABLED; } +let numEmberVersion = parseFloat(ENV._OVERRIDE_DEPRECATION_VERSION ?? VERSION); + +/* until must only be a minor version or major version */ +export function emberVersionGte(until: string, emberVersion = numEmberVersion) { + let significantUntil = until.replace(/(\.0+)/g, ''); + return emberVersion >= parseFloat(significantUntil); +} + +export function isRemoved(options: DeprecationOptions) { + return emberVersionGte(options.until); +} + +interface DeprecationObject { + options: DeprecationOptions; + test: boolean; + isEnabled: boolean; + isRemoved: boolean; +} + function deprecation(options: DeprecationOptions) { return { options, test: !isEnabled(options), isEnabled: isEnabled(options), + isRemoved: isRemoved(options), }; } @@ -39,7 +61,7 @@ function deprecation(options: DeprecationOptions) { import { DEPRECATIONS } from '@ember/-internals/deprecations'; //... - deprecate(message, DEPRECATIONS.MY_DEPRECATION.test, DEPRECATIONS.MY_DEPRECATION.options); + deprecateUntil(message, DEPRECATIONS.MY_DEPRECATION); ``` `expectDeprecation` should also use the DEPRECATIONS object, but it should be noted @@ -55,6 +77,17 @@ function deprecation(options: DeprecationOptions) { DEPRECATIONS.MY_DEPRECATION.isEnabled ); ``` + + Tests can be conditionally run based on whether a deprecation is enabled or not: + + ```ts + [`${testUnless(DEPRECATIONS.MY_DEPRECATION.isRemoved)} specific deprecated feature tested only in this test`] + ``` + + This test will be skipped when the MY_DEPRECATION is removed. + When adding a deprecation, we need to guard all the code that will eventually be removed, including tests. + For tests that are not specifically testing the deprecated feature, we need to figure out how to + test the behavior without encountering the deprecated feature, just as users would. */ export const DEPRECATIONS = { DEPRECATE_IMPLICIT_ROUTE_MODEL: deprecation({ @@ -65,3 +98,17 @@ export const DEPRECATIONS = { url: 'https://deprecations.emberjs.com/v5.x/#toc_deprecate-implicit-route-model', }), }; + +export function deprecateUntil(message: string, deprecation: DeprecationObject) { + const { options } = deprecation; + assert( + 'deprecateUntil must only be called for ember-source', + Boolean(options.for === 'ember-source') + ); + if (deprecation.isRemoved) { + throw new Error( + `The API deprecated by ${options.id} was removed in ember-source ${options.until}. The message was: ${message}. Please see ${options.url} for more details.` + ); + } + deprecate(message, deprecation.test, options); +} diff --git a/packages/@ember/-internals/deprecations/tests/index-test.js b/packages/@ember/-internals/deprecations/tests/index-test.js new file mode 100644 index 00000000000..5d3d0efea36 --- /dev/null +++ b/packages/@ember/-internals/deprecations/tests/index-test.js @@ -0,0 +1,137 @@ +import { AbstractTestCase, moduleFor } from 'internal-test-helpers'; +import { deprecateUntil, isRemoved, emberVersionGte } from '../index'; +import { ENV } from '@ember/-internals/environment'; + +let originalEnvValue; + +moduleFor( + '@ember/-internals/deprecations', + class extends AbstractTestCase { + constructor() { + super(); + originalEnvValue = ENV.RAISE_ON_DEPRECATION; + ENV.RAISE_ON_DEPRECATION = false; + } + + teardown() { + ENV.RAISE_ON_DEPRECATION = originalEnvValue; + } + + ['@test deprecateUntil throws when deprecation has been removed'](assert) { + assert.expect(1); + + let MY_DEPRECATION = { + options: { + id: 'test', + until: '3.0.0', + for: 'ember-source', + url: 'http://example.com/deprecations/test', + since: { + available: '1.0.0', + enabled: '1.0.0', + }, + }, + isRemoved: true, + }; + + assert.throws( + () => deprecateUntil('Old long gone api is deprecated', MY_DEPRECATION), + /Error: The API deprecated by test was removed in ember-source 3.0.0. The message was: Old long gone api is deprecated. Please see http:\/\/example.com\/deprecations\/test for more details./, + 'deprecateUntil throws when isRemoved is true on deprecation' + ); + } + + ['@test deprecateUntil does not throw when isRemoved is false on deprecation'](assert) { + assert.expect(1); + + let MY_DEPRECATION = { + options: { + id: 'test', + until: '3.0.0', + for: 'ember-source', + url: 'http://example.com/deprecations/test', + since: { + available: '1.0.0', + enabled: '1.0.0', + }, + }, + isRemoved: false, + }; + + deprecateUntil('Deprecation is thrown', MY_DEPRECATION); + + assert.ok(true, 'exception on until was not thrown'); + } + ['@test isRemoved is true when until has passed current ember version'](assert) { + assert.expect(1); + + let options = { + id: 'test', + until: '3.0.0', + for: 'ember-source', + url: 'http://example.com/deprecations/test', + since: { available: '1.0.0', enabled: '1.0.0' }, + }; + + assert.strictEqual(isRemoved(options), true, 'isRemoved is true when until has passed'); + } + + ['@test isRemoved is false before until has passed current ember version'](assert) { + assert.expect(1); + + let options = { + id: 'test', + until: '30.0.0', + for: 'ember-source', + url: 'http://example.com/deprecations/test', + since: { available: '1.0.0', enabled: '1.0.0' }, + }; + + assert.strictEqual( + isRemoved(options), + false, + 'isRemoved is false until the until has passed' + ); + } + + ['@test emberVersionGte returns whether the ember version is greater than or equal to the provided version']( + assert + ) { + assert.strictEqual( + emberVersionGte('3.0.0', parseFloat('5.0.0')), + true, + '5.0.0 is after 3.0.0' + ); + assert.strictEqual( + emberVersionGte('30.0.0', parseFloat('5.0.0')), + false, + '5.0.0 is before 30.0.0' + ); + assert.strictEqual( + emberVersionGte('5.0.0-beta.1', parseFloat('5.0.0')), + true, + '5.0.0 is after 5.0.0-beta.1' + ); + assert.strictEqual( + emberVersionGte('5.0.1', parseFloat('5.0.0-beta.1')), + false, + '5.0.0-beta.1 is before 5.0.1' + ); + assert.strictEqual( + emberVersionGte('5.0.0-alpha.abcde', parseFloat('5.0.0')), + true, + '5.0.0 is after 5.0.0-alpha' + ); + assert.strictEqual( + emberVersionGte('5.9.0', parseFloat('5.8.9')), + false, + '5.8.9 is before 5.9.0' + ); + assert.strictEqual( + emberVersionGte('5.10.0', parseFloat('5.9.2')), + true, + '5.10.1 is after 5.9.2' + ); + } + } +); diff --git a/packages/@ember/-internals/environment/lib/env.ts b/packages/@ember/-internals/environment/lib/env.ts index ea44b7d1d7d..bef9f01d815 100644 --- a/packages/@ember/-internals/environment/lib/env.ts +++ b/packages/@ember/-internals/environment/lib/env.ts @@ -141,6 +141,18 @@ export const ENV = { */ _ALL_DEPRECATIONS_ENABLED: false, + /** + Override the version of ember-source used to determine when deprecations "break". + This is used internally by Ember to test with deprecated features "removed". + This is never intended to be set by projects. + @property _OVERRIDE_DEPRECATION_VERSION + @for EmberENV + @type string | null + @default null + @private + */ + _OVERRIDE_DEPRECATION_VERSION: null, + /** Whether the app defaults to using async observers. @@ -214,6 +226,8 @@ export const ENV = { (ENV as Record)[flag] = EmberENV[flag] !== false; } else if (defaultValue === false) { (ENV as Record)[flag] = EmberENV[flag] === true; + } else { + (ENV as Record)[flag] = EmberENV[flag]; } } diff --git a/packages/@ember/routing/route.ts b/packages/@ember/routing/route.ts index 3ffee516eb2..62b51e9c6c8 100644 --- a/packages/@ember/routing/route.ts +++ b/packages/@ember/routing/route.ts @@ -18,8 +18,8 @@ import { isProxy, lookupDescriptor } from '@ember/-internals/utils'; import type { AnyFn } from '@ember/-internals/utility-types'; import Controller from '@ember/controller'; import type { ControllerQueryParamType } from '@ember/controller'; -import { assert, deprecate, info, isTesting } from '@ember/debug'; -import { DEPRECATIONS } from '@ember/-internals/deprecations'; +import { assert, info, isTesting } from '@ember/debug'; +import { DEPRECATIONS, deprecateUntil } from '@ember/-internals/deprecations'; import EngineInstance from '@ember/engine/instance'; import { dependentKeyCompat } from '@ember/object/compat'; import { once } from '@ember/runloop'; @@ -1256,11 +1256,10 @@ class Route extends EmberObject.extend(ActionHandler, Evented) if (ENV._NO_IMPLICIT_ROUTE_MODEL) { return; } - deprecate( + deprecateUntil( `The implicit model loading behavior for routes is deprecated. ` + `Please define an explicit model hook for ${this.fullRouteName}.`, - DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.test, - DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.options + DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL ); const store = 'store' in this ? this.store : get(this, '_store'); diff --git a/packages/@ember/routing/tests/system/route_test.js b/packages/@ember/routing/tests/system/route_test.js index b3e2c02140b..38b1bb14544 100644 --- a/packages/@ember/routing/tests/system/route_test.js +++ b/packages/@ember/routing/tests/system/route_test.js @@ -1,7 +1,13 @@ import { setOwner } from '@ember/-internals/owner'; import { ENV } from '@ember/-internals/environment'; import { DEPRECATIONS } from '@ember/-internals/deprecations'; -import { runDestroy, buildOwner, moduleFor, AbstractTestCase } from 'internal-test-helpers'; +import { + runDestroy, + buildOwner, + moduleFor, + testUnless, + AbstractTestCase, +} from 'internal-test-helpers'; import Service, { service } from '@ember/service'; import EmberObject from '@ember/object'; import EmberRoute from '@ember/routing/route'; @@ -35,7 +41,9 @@ moduleFor( ENV._NO_IMPLICIT_ROUTE_MODEL = this._NO_IMPLICIT_ROUTE_MODEL; } - ['@test default store utilizes the container to acquire the model factory'](assert) { + [`${testUnless( + DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.isRemoved + )} default store utilizes the container to acquire the model factory`](assert) { assert.expect(5); let Post = EmberObject.extend(); @@ -69,11 +77,10 @@ moduleFor( setOwner(route, owner); expectDeprecation( - () => - ignoreAssertion(() => { - assert.equal(route.model({ post_id: 1 }), post); - assert.equal(route.findModel('post', 1), post, '#findModel returns the correct post'); - }), + () => { + assert.equal(route.model({ post_id: 1 }), post); + assert.equal(route.findModel('post', 1), post, '#findModel returns the correct post'); + }, /The implicit model loading behavior for routes is deprecated./, DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.isEnabled ); @@ -97,7 +104,9 @@ moduleFor( assert.true(calledFind, 'store.find was called'); } - ["@test assert if 'store.find' method is not found"]() { + [`${testUnless( + DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.isRemoved + )} assert if 'store.find' method is not found`]() { runDestroy(route); let owner = buildOwner(); @@ -134,7 +143,9 @@ moduleFor( runDestroy(owner); } - ['@test asserts if model class is not found']() { + [`${testUnless( + DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.isRemoved + )} asserts if model class is not found`]() { runDestroy(route); let owner = buildOwner(); @@ -161,7 +172,9 @@ moduleFor( runDestroy(owner); } - ["@test 'store' does not need to be injected"](assert) { + [`${testUnless( + DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.isRemoved + )} 'store' does not need to be injected`](assert) { runDestroy(route); let owner = buildOwner(); diff --git a/packages/ember/tests/routing/decoupled_basic_test.js b/packages/ember/tests/routing/decoupled_basic_test.js index 69b07d43d27..9e1888876d4 100644 --- a/packages/ember/tests/routing/decoupled_basic_test.js +++ b/packages/ember/tests/routing/decoupled_basic_test.js @@ -201,28 +201,21 @@ moduleFor( } ["@test The Special page returning an error invokes SpecialRoute's error handler"](assert) { + assert.expect(2); + this.router.map(function () { this.route('home', { path: '/' }); - this.route('special', { path: '/specials/:menu_item_id' }); - }); - - let menuItem, promise, resolve; - - let MenuItem = EmberObject.extend(); - MenuItem.reopenClass({ - find(id) { - menuItem = MenuItem.create({ id: id }); - promise = new RSVP.Promise((res) => (resolve = res)); - - return promise; - }, + this.route('special', { path: '/specials/:menu_item' }); }); - this.add('model:menu_item', MenuItem); + let resolve; this.add( 'route:special', Route.extend({ + model() { + return new RSVP.Promise((res) => (resolve = res)); + }, setup() { throw new Error('Setup error'); }, @@ -239,11 +232,9 @@ moduleFor( }) ); - ignoreDeprecation(() => { - runTask(() => handleURLRejectsWith(this, assert, 'specials/1', 'Setup error')); - }); + runTask(() => handleURLRejectsWith(this, assert, 'specials/1', 'Setup error')); - resolve(menuItem); + resolve(); } ["@test ApplicationRoute's default error handler can be overridden"](assert) { diff --git a/packages/internal-test-helpers/index.ts b/packages/internal-test-helpers/index.ts index 5695bf2329f..7c56e643030 100644 --- a/packages/internal-test-helpers/index.ts +++ b/packages/internal-test-helpers/index.ts @@ -18,6 +18,7 @@ export { defineSimpleHelper, defineSimpleModifier, } from './lib/define-template-values'; +export { testIf, testUnless } from './lib/conditional-test'; export { default as compile } from './lib/compile'; export { equalsElement, classes, styles, regex } from './lib/matchers'; export { runAppend, runDestroy, runTask, runTaskNext, runLoopSettled } from './lib/run'; diff --git a/packages/internal-test-helpers/lib/conditional-test.ts b/packages/internal-test-helpers/lib/conditional-test.ts new file mode 100644 index 00000000000..db022803217 --- /dev/null +++ b/packages/internal-test-helpers/lib/conditional-test.ts @@ -0,0 +1,9 @@ +function testIf(condition: boolean) { + return condition ? '@test' : '@skip'; +} + +function testUnless(condition: boolean) { + return testIf(!condition); +} + +export { testIf, testUnless }; diff --git a/tests/docs/expected.js b/tests/docs/expected.js index 8a9ec0603f3..f781f6fe1b7 100644 --- a/tests/docs/expected.js +++ b/tests/docs/expected.js @@ -14,6 +14,7 @@ module.exports = { '_RERENDER_LOOP_LIMIT', '_TEMPLATE_ONLY_GLIMMER_COMPONENTS', '_ALL_DEPRECATIONS_ENABLED', + '_OVERRIDE_DEPRECATION_VERSION', 'Input', 'LinkTo', 'Textarea', diff --git a/tests/index.html b/tests/index.html index 2c228bb5eb1..97c3ba250b6 100644 --- a/tests/index.html +++ b/tests/index.html @@ -58,6 +58,10 @@ if (QUnit.urlParams.alldeprecationsenabled) { EmberENV['_ALL_DEPRECATIONS_ENABLED'] = QUnit.urlParams.alldeprecationsenabled; } + + if (QUnit.urlParams.overridedeprecationversion) { + EmberENV['_OVERRIDE_DEPRECATION_VERSION'] = QUnit.urlParams.overridedeprecationversion; + } })();