-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thegilby Yeah, I'm following up with updates to the the API docs. |
Added usage info to API docs. |
There was a problem hiding this 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!
I incorporated the requested changes yesterday and updated the associated TSDoc comments so that things get published out properly to the |
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! 🙏 |
@simonihmig sorry for the delay; let's get #1389 in as well early next week and get a release out! |
export { | ||
registerHook as _registerHook, | ||
runHooks as _runHooks, | ||
} from './-internal/helper-hooks'; | ||
registerHook, | ||
runHooks, | ||
type Hook, | ||
type HookLabel, | ||
type HookUnregister, | ||
} from './helper-hooks'; |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
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.