From 98af2cd3d88b1c94e69f04114a6619bd25d6d576 Mon Sep 17 00:00:00 2001 From: Katie Gengler Date: Tue, 26 Mar 2024 11:37:43 -0400 Subject: [PATCH 1/2] Run all tests with all deprecations enabled, and with them at the default state for the version - Create a listing of deprecations that should be used as the source of truth for deprecations issued by Ember itself - Convention to pass the test/isEnabled to deprecate/expectDeprecation so that our testing mechanism will work --- .github/workflows/ci.yml | 23 +++++++++++++++++++ bin/run-tests.js | 4 ++++ .../@ember/-internals/deprecations/index.ts | 23 +++++++++++++++++++ .../@ember/-internals/environment/lib/env.ts | 13 +++++++++++ packages/@ember/routing/route.ts | 11 +++------ .../@ember/routing/tests/system/route_test.js | 4 +++- tests/docs/expected.js | 1 + tests/index.html | 4 ++++ 8 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 packages/@ember/-internals/deprecations/index.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c3aea82d05..faa7c0d9ca8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -79,6 +79,29 @@ jobs: TEST_SUITE: each-package run: pnpm test + deprecations-enabled-test: + name: Debug and Prebuilt (All Tests by Package + Canary Features) with all Deprecations enabled + 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 + ALL_DEPRECATIONS_ENABLED: true + run: pnpm test + browserstack-test: name: Browserstack Tests (Safari, Edge) runs-on: ubuntu-latest diff --git a/bin/run-tests.js b/bin/run-tests.js index cbd49c87fc8..911efb31e96 100755 --- a/bin/run-tests.js +++ b/bin/run-tests.js @@ -39,6 +39,10 @@ function run(queryString) { queryString = `${queryString}&debugrendertree`; } + if (process.env.ALL_DEPRECATIONS_ENABLED) { + queryString = `${queryString}&alldeprecationsenabled=${process.env.ALL_DEPRECATIONS_ENABLED}`; + } + 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 new file mode 100644 index 00000000000..07d864c7393 --- /dev/null +++ b/packages/@ember/-internals/deprecations/index.ts @@ -0,0 +1,23 @@ +import type { DeprecationOptions } from '@ember/debug/lib/deprecate'; +import { ENV } from '@ember/-internals/environment'; + +function isEnabled(options: DeprecationOptions) { + return Object.hasOwnProperty.call(options.since, 'enabled') || ENV._ALL_DEPRECATIONS_ENABLED; +} + +function deprecation(options: DeprecationOptions) { + return { + options, + test: !isEnabled(options), + isEnabled: isEnabled(options), + }; +} +export const DEPRECATIONS = { + DEPRECATE_IMPLICIT_ROUTE_MODEL: deprecation({ + id: 'deprecate-implicit-route-model', + for: 'ember-source', + since: { available: '5.3.0', enabled: '5.3.0' }, + until: '6.0.0', + url: 'https://deprecations.emberjs.com/v5.x/#toc_deprecate-implicit-route-model', + }), +}; diff --git a/packages/@ember/-internals/environment/lib/env.ts b/packages/@ember/-internals/environment/lib/env.ts index 6008a14e936..ea44b7d1d7d 100644 --- a/packages/@ember/-internals/environment/lib/env.ts +++ b/packages/@ember/-internals/environment/lib/env.ts @@ -128,6 +128,19 @@ export const ENV = { */ _DEBUG_RENDER_TREE: DEBUG, + /** + Whether to force all deprecations to be enabled. This is used internally by + Ember to enable deprecations in tests. It is not intended to be set in + projects. + + @property _ALL_DEPRECATIONS_ENABLED + @for EmberENV + @type Boolean + @default false + @private + */ + _ALL_DEPRECATIONS_ENABLED: false, + /** Whether the app defaults to using async observers. diff --git a/packages/@ember/routing/route.ts b/packages/@ember/routing/route.ts index 8a060e23747..3ffee516eb2 100644 --- a/packages/@ember/routing/route.ts +++ b/packages/@ember/routing/route.ts @@ -19,6 +19,7 @@ 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 EngineInstance from '@ember/engine/instance'; import { dependentKeyCompat } from '@ember/object/compat'; import { once } from '@ember/runloop'; @@ -1258,14 +1259,8 @@ class Route extends EmberObject.extend(ActionHandler, Evented) deprecate( `The implicit model loading behavior for routes is deprecated. ` + `Please define an explicit model hook for ${this.fullRouteName}.`, - false, - { - id: 'deprecate-implicit-route-model', - for: 'ember-source', - since: { available: '5.3.0', enabled: '5.3.0' }, - until: '6.0.0', - url: 'https://deprecations.emberjs.com/v5.x/#toc_deprecate-implicit-route-model', - } + DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.test, + DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.options ); 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 072192a567b..b3e2c02140b 100644 --- a/packages/@ember/routing/tests/system/route_test.js +++ b/packages/@ember/routing/tests/system/route_test.js @@ -1,5 +1,6 @@ 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 Service, { service } from '@ember/service'; import EmberObject from '@ember/object'; @@ -73,7 +74,8 @@ moduleFor( 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./ + /The implicit model loading behavior for routes is deprecated./, + DEPRECATIONS.DEPRECATE_IMPLICIT_ROUTE_MODEL.isEnabled ); runDestroy(owner); diff --git a/tests/docs/expected.js b/tests/docs/expected.js index d02bb5986d7..8a9ec0603f3 100644 --- a/tests/docs/expected.js +++ b/tests/docs/expected.js @@ -13,6 +13,7 @@ module.exports = { '_NO_IMPLICIT_ROUTE_MODEL', '_RERENDER_LOOP_LIMIT', '_TEMPLATE_ONLY_GLIMMER_COMPONENTS', + '_ALL_DEPRECATIONS_ENABLED', 'Input', 'LinkTo', 'Textarea', diff --git a/tests/index.html b/tests/index.html index 0b3f29fe8c1..2c228bb5eb1 100644 --- a/tests/index.html +++ b/tests/index.html @@ -54,6 +54,10 @@ if (QUnit.urlParams.debugrendertree) { EmberENV['_DEBUG_RENDER_TREE'] = true; } + + if (QUnit.urlParams.alldeprecationsenabled) { + EmberENV['_ALL_DEPRECATIONS_ENABLED'] = QUnit.urlParams.alldeprecationsenabled; + } })(); From 7105a4ba3fce1c12d9df17eb63728fbd8a3f879a Mon Sep 17 00:00:00 2001 From: Katie Gengler Date: Tue, 26 Mar 2024 12:24:01 -0400 Subject: [PATCH 2/2] Document new deprecations convention - Updates to CONTRIBUTING.md --- CONTRIBUTING.md | 108 +++++++++++++----- .../@ember/-internals/deprecations/index.ts | 44 +++++++ 2 files changed, 124 insertions(+), 28 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ba652f57c62..cd79e2912fe 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -5,10 +5,6 @@ to collect and track bugs and discussions of new features. If you are having difficulties using Ember.js or have a question about usage, please ask a question on Stack Overflow: http://stackoverflow.com/questions/ask?tags=ember.js, or by joining [the community chat](https://discord.gg/emberjs). -The Ember.js community is very active on Stack Overflow and most questions -receive attention the same day they're posted: -http://stackoverflow.com/questions/tagged/ember.js - # Issue Labeling Ember uses [StandardIssueLabels](https://github.com/wagenet/StandardIssueLabels) for Github Issues. @@ -24,12 +20,12 @@ fixed your bug. 2. Search for similar issues. It's possible somebody has encountered this bug already. -3. Provide [Ember Twiddle](http://ember-twiddle.com/) demo that specifically -shows the problem. This demo should be fully operational with the exception -of the bug you want to demonstrate. The more pared down, the better. -If it is not possible to produce a fiddle, please make sure you provide very -specific steps to reproduce the error. If we cannot reproduce it, we will -close the ticket. +3. Provide a demo that specifically shows the problem. This demo should be fully +operational with the exception of the bug you want to demonstrate. +You may provide a repo or use a fork on [new.emberjs.com](http://new.emberjs.com). +The more pared down, the better. If it is not possible to produce a demo, please +make sure you provide very specific steps to reproduce the error. If we cannot +reproduce it, we will close the ticket. 4. Your issue will be verified. The provided example will be tested for correctness. The Ember team will work with you until your issue can @@ -66,12 +62,8 @@ and the issue will be closed ## Requesting a Feature -1. Ember has an RFC process for feature requests. To begin the discussion either -[gather feedback](https://github.com/emberjs/rfcs/blob/master/README.md#gathering-feedback-before-submitting) -on the emberjs/rfcs repository. Or, draft an [Ember RFC](https://github.com/emberjs/rfcs#ember-rfcs) - - Use RFC pull request for well formed ideas. - - Use RFC issues to propose a rough idea, basically a great place to test - the waters. +1. Ember has an RFC process for feature requests. To begin the discussion +[follow the guidance on the RFC repo](https://github.com/emberjs/rfcs/blob/master/README.md#creating-an-rfc). 2. Provide a clear and detailed explanation of the feature you want and why it's important to add. Keep in mind that we want features that will be useful @@ -158,6 +150,7 @@ To run a specific browser, you can use the `--launch` flag * `ember test --launch SL_Firefox_Current,Chrome` To test multiple launchers, you can separate them with commas. + # Pull Requests We love pull requests. Here's a quick guide: @@ -198,11 +191,10 @@ explanation of why you made the changes you made. For new features make sure to explain a standard use case to us. We try to be quick about responding to tickets but sometimes we get a bit -backlogged. If the response is slow, try to find someone on IRC (#emberjs) to +backlogged. If the response is slow, try to find someone on [the community chat](https://discord.gg/emberjs) to give the ticket a review. -Some things that will increase the chance that your pull request is accepted, -taken straight from the Ruby on Rails guide: +Some things that will increase the chance that your pull request is accepted: * Use Ember idioms and helpers * Include tests that fail without your code, and pass with it @@ -211,9 +203,7 @@ taken straight from the Ruby on Rails guide: Syntax: -* Two spaces, no tabs. -* No trailing whitespace. Blank lines should not have any space. -* a = b and not a=b. +* Style is enforced by prettier. Run `pnpm lint` to check your changes. * Follow the conventions you see used in the source already. Inline Documentation Guidelines: @@ -251,11 +241,23 @@ We use [GitHub Actions](https://github.com/emberjs/ember.js/actions?query=workfl When you submit your PR (or later change that code), a CI build will automatically be kicked off. A note will be added to the PR, and will indicate the current status of the build. -Within the CI build, you can see that we (currently) run six different test suites. +Within the [CI workflow](https://github.com/emberjs/ember.js/blob/main/.github/workflows/ci.yml), you can see that we (currently) run several different test suites. + +* `Linting` runs `pnpm lint` to check for code style issues. +* `Type Checking` runs `pnpm type-check` to check for TypeScript type errors. This also runs against several version of TypeScript that are supported. +* `Basic Test` / `each-package` test suite is closest to what you normally run locally on your machine. This is also run with all deprecations enabled -- that is, a deprecation that has been added but is not yet up to it's enabled version will be forced "on" and the test suite is required to pass both ways. +* `Production` test suite runs tests against a production build. This also runs with the "Debug Render Tree" feature enabled. +* `BrowserStack` and `Browser Tests` test suites run tests against various supported browsers. +* `Blueprint Tests` runs tests for the Ember CLI blueprints provided by Ember in this package. +* `Smoke Test` builds and runs a simple app. +* `Extend Prototypes` runs the `extend-prototypes` test suite. +* `Node.js Tests` runs tests for the node-side code in this package. -* The `each-package` test suite is closest to what you normally run locally on your machine. -* The `build-tests EMBER_ENV=production...` test suite runs tests against a production build. -* The `browserstack` test suite runs tests against various supported browsers. +Each commit to canary and beta publishes to the relevant channel on S3. These +builds are used primarily by [ember-try](https://github.com/ember-cli/ember-try) +for addons to test against beta and canary of ember-source. + +The CI workflow is also run nightly for beta and canary. ## Common CI Build Issues @@ -265,7 +267,7 @@ If your build is failing on the 'production' suite, you may be relying on a debu There are helpers for many of these functions, which will resolve this for you: `expectDeprecation`, `expectAssertion`, etc. Please use these helpers when dealing with these functions. -If your tests can't aren't covered a helper, one common solution is the use of `DEBUG` flag. Wrapping the debug-only dependent test in a check of this flag will cause that test to not be run in the prod test suite: +If your tests can't or aren't covered by a helper, one common solution is the use of `DEBUG` flag. Wrapping the debug-only dependent test in a check of this flag will cause that test to not be run in the prod test suite: ```javascript import { DEBUG } from '@glimmer/env'; @@ -287,6 +289,56 @@ Sometimes a single test suite will fail, without giving any indication of a real * Restart all the test suites on CI by doing another push * Ask a repo collab to restart that single test suite +# Adding a Deprecation + +Deprecations of public API must be the result of an [RFC](https://github.com/emberjs/rfcs#creating-an-rfc). + +Deprecations are defined in a central location, currently [packages/@ember/-internals/deprecation/index.ts](https://github.com/emberjs/ember.js/blob/main/packages/%40ember/-internals/deprecation/index.ts). + +To add a deprecation, you must add a new entry to the `deprecations` object in the `index.ts` file. The entry should be an object with the following properties: + +* `id` (required): A string that uniquely identifies the deprecation. This should be a short, descriptive name, typically dasherized. +* `for` (required): The string `ember-source` -- every deprecation from this package is for `ember-source`. +* `since` (required): An object with `available` and `enabled`. `available` is the +first version of Ember that the deprecation is available in. `enabled` is the version +of Ember that the deprecation was first enabled. This is used as a feature flag +deprecations. For public APIs, the `enabled` value is added only once the deprecation RFC is [Ready for Release](https://github.com/emberjs/rfcs#ready-for-release). +* `until` (required): The version of Ember that the deprecation will be removed in. +* `url` (required): A URL to the deprecation guide for the deprecation. This URL +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. + +```ts +import { DEPRECATIONS } from '@ember/-internals/deprecations'; +//... + +deprecate(message, DEPRECATIONS.MY_DEPRECATION.test, DEPRECATIONS.MY_DEPRECATION.options); + +``` + +`expectDeprecation` should also use the DEPRECATIONS object, but it should be noted +that it uses `isEnabled` instead of `test` because the expectations of `expectDeprecation` +are the opposite of `test`. + +```ts + expectDeprecation( + () => { + assert.equal(foo, bar(), 'foo is equal to bar'); // something that triggers the deprecation + }, + /matchesMessage/, + DEPRECATIONS.MY_DEPRECATION.isEnabled +); +``` + +We sometimes deprecate "Intimate" API -- private APIs that we suspect or have +concerns about being used by addons or apps. These deprecations are not required +to be RFC'd. However, we require they be available for at least one LTS release +before the APIs are removed. For example, a deprecation added in 5.5 would need +to have an `until` no earlier than 5.9. + # Appendix ## Commit Tagging @@ -308,7 +360,7 @@ as `[CLEANUP beta]`. ### Features -All additions and fixes for features in canary should be tagged as `[FEATURE name]` where name is the same as the flag for that feature. +All additions and fixes for features in canary should be tagged as `[FEATURE name]` where name is the same as the flag for that feature. The commit message or PR description should link to the RFC that proposed the feature. ### Documentation diff --git a/packages/@ember/-internals/deprecations/index.ts b/packages/@ember/-internals/deprecations/index.ts index 07d864c7393..129dd745a43 100644 --- a/packages/@ember/-internals/deprecations/index.ts +++ b/packages/@ember/-internals/deprecations/index.ts @@ -12,6 +12,50 @@ function deprecation(options: DeprecationOptions) { isEnabled: isEnabled(options), }; } + +/* + To add a deprecation, you must add a new entry to the `DEPRECATIONS` object. + The entry should be an object with the following properties: + + * `id` (required): A string that uniquely identifies the deprecation. This + should be a short, descriptive name, typically dasherized. + * `for` (required): The string `ember-source` -- every deprecation from this + package is for `ember-source`. + * `since` (required): An object with `available` and `enabled`. `available` is + the first version of Ember that the deprecation is available in. `enabled` is + the version of Ember that the deprecation was first enabled. This is used as + a feature flag deprecations. For public APIs, the `enabled` value is added + only once the deprecation RFC is [Ready for Release](https://github.com/emberjs/rfcs#ready-for-release). + * `until` (required): The version of Ember that the deprecation will be removed + * `url` (required): A URL to the deprecation guide for the deprecation. This + URL 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}}`. + + For example: + `deprecate` should then be called using the entry from the `DEPRECATIONS` object. + + ```ts + import { DEPRECATIONS } from '@ember/-internals/deprecations'; + //... + + deprecate(message, DEPRECATIONS.MY_DEPRECATION.test, DEPRECATIONS.MY_DEPRECATION.options); + ``` + + `expectDeprecation` should also use the DEPRECATIONS object, but it should be noted + that it uses `isEnabled` instead of `test` because the expectations of `expectDeprecation` + are the opposite of `test`. + + ```ts + expectDeprecation( + () => { + assert.equal(foo, bar(), 'foo is equal to bar'); // something that triggers the deprecation + }, + /matchesMessage/, + DEPRECATIONS.MY_DEPRECATION.isEnabled + ); + ``` + */ export const DEPRECATIONS = { DEPRECATE_IMPLICIT_ROUTE_MODEL: deprecation({ id: 'deprecate-implicit-route-model',