Skip to content
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

Expose registerHook and runHooks as official public APIs #1344

Merged
merged 4 commits into from
Mar 8, 2023

Conversation

drewlee
Copy link
Contributor

@drewlee drewlee commented Mar 1, 2023

Addresses issue #1341. This change exposes registerHook, runHooks, and the associated types as official public API. The update essentially moves helper hooks out of -internal/ and modifies the associated imports.

- Verified addon builds and TS compiles properly.
- Tests pass.
- Verified hooks functionality and types via ember-a11y-testing.
- Verified published updates to API.md. See screenshot below.

Screenshot 2023-03-07 at 10 48 54 AM

@drewlee drewlee marked this pull request as ready for review March 1, 2023 23:05
@drewlee drewlee changed the title [WIP] Expose registerHook and runHooks as official public APIs Expose registerHook and runHooks as official public APIs Mar 1, 2023
Copy link

@thegilby thegilby left a comment

Choose a reason for hiding this comment

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

Thanks for upgrading these @drewlee!

Would it be helpful to update the API docs to include these?

@drewlee
Copy link
Contributor Author

drewlee commented Mar 2, 2023

@thegilby Yeah, I'm following up with updates to the the API docs.

@drewlee
Copy link
Contributor Author

drewlee commented Mar 2, 2023

Added usage info to API docs.

@drewlee
Copy link
Contributor Author

drewlee commented Mar 2, 2023

@chriskrycho @scalvert

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

This needs one set of changes (which you should be able to apply directly as a batch from the Files tab on this PR, if I’ve done things correctly here!) to account for a way the existing types misled you as you were working on this.

Thanks for getting this done!

addon-test-support/@ember/test-helpers/helper-hooks.ts Outdated Show resolved Hide resolved
addon-test-support/@ember/test-helpers/helper-hooks.ts Outdated Show resolved Hide resolved
addon-test-support/@ember/test-helpers/helper-hooks.ts Outdated Show resolved Hide resolved
addon-test-support/@ember/test-helpers/helper-hooks.ts Outdated Show resolved Hide resolved
addon-test-support/@ember/test-helpers/index.ts Outdated Show resolved Hide resolved
type-tests/api.ts Outdated Show resolved Hide resolved
type-tests/api.ts Outdated Show resolved Hide resolved
type-tests/api.ts Outdated Show resolved Hide resolved
API.md Outdated Show resolved Hide resolved
@drewlee drewlee requested a review from chriskrycho March 6, 2023 20:35
@drewlee
Copy link
Contributor Author

drewlee commented Mar 7, 2023

I incorporated the requested changes yesterday and updated the associated TSDoc comments so that things get published out properly to the API.md doc (see the screenshot in the PR description). I think this is good to go, would appreciate another look @chriskrycho. Thanks!

@chriskrycho chriskrycho merged commit a0b6f84 into emberjs:master Mar 8, 2023
@simonihmig
Copy link
Contributor

Thanks for getting this fixed! @chriskrycho may I ask you for a new release, so that we can get ember-a11y/ember-a11y-testing#498 unblocked? Thank you! 🙏

@chriskrycho
Copy link
Contributor

@simonihmig sorry for the delay; let's get #1389 in as well early next week and get a release out!

Comment on lines 52 to +58
export {
registerHook as _registerHook,
runHooks as _runHooks,
} from './-internal/helper-hooks';
registerHook,
runHooks,
type Hook,
type HookLabel,
type HookUnregister,
} from './helper-hooks';
Copy link
Member

Choose a reason for hiding this comment

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

As is, this will break any current users of ember-a11y-testing (and will require ember-a11y-testing to do a major release that only works with @ember/test-helpers@2.10.0 or whatever this PR is released as).

We should add the following (to this file) BEFORE releasing:

export function _registerHook(...args) {
  deprecate('something good here', { ... });
  return registerHook(...args);
}

export function _runHooks(...args) {
  deprecate('something good here', { ... });
  return runHooks(...args);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@rwjblue I think a new release of both will resolve the issue, yes? Coordinating a major version bump for this package now, and then it can be updated in ember-a11y-testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MelSumner we absolutely do not want to do a major of this right now. We have other TS things in flight which we want to land backwards-compatibly before that. This doesn't need a breaking change to release, it just needs a deprecation-and-not-removal here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants