adopted-ember-addons / ember-cli-flash

Simple, highly configurable flash messages for ember-cli
https://www.npmjs.com/package/ember-cli-flash
MIT License
356 stars 113 forks source link

Replace blueprint with explicit test helpers #392

Closed gilest closed 9 months ago

gilest commented 9 months ago

Old world 🌐

Previously the following was added to an Ember CLI app when installed via ember install ember-cli-flash.

// tests/helpers/flash-message.js
import FlashObject from 'ember-cli-flash/flash/object';

FlashObject.reopen({ init() {} });
// tests/test-helper.js
import './helpers/flash-message';

Essentially importing this helper would cause the init hook of the FlashObject to become a no-op and disable the timeout running in consuming apps test runs.

It wasn't very explicit or obvious that what was happening as a result of this import. Looking at apps that I maintain, some do and some don't have this...

New world 🌏

Blueprints are not supported in V2 addons, so this PR ships two new test helpers instead.

import { disableTimeout, enableTimeout } from 'ember-cli-flash/test-support';

To get the same behaviour as "Old world 🌐 " a consuming app needs only to remove the blueprint import. βœ…

However this can be changed at any time with the enableTimers function. This allows consuming apps to choose on a per-test basis whether Flash timeouts should occur – as demonstrated by the included tests.

Why not disable timeouts for all test runs? πŸ€”

There are many existing apps with test suites that both do and do not have timeouts disabled at present I think it's important that this is configurable for compatibility reasons.

Even this addon's own test suite tests with the timeout functionality enabled.

Other options 🀷🏻

Very open to suggestions here. Just wanted to have a known way forward to replace the existing blueprint.

Documentation

gilest commented 9 months ago

Self-reivew: I think we can reduce the configuration a bit here by disabling timers by default.

Edit: Yeah – adjusted so that there's no installation config needed and the upgrading steps are simpler for most users (remove a single import).

gilest commented 9 months ago

@NullVoxPopuli please add breaking