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

refactor(accounts-controller)!: revert wildcard export #5300

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Feb 6, 2025

Explanation

Using a wildcard export in the root index.ts of a package is, regrettably, not an ideal practice. This file is very useful because it gives us a way to understand everything that a package publicly exports at a glance.

In using a wildcard export, however, we lose this visibility. Because all symbols marked with export will automatically become exports of the package, it is possible to introduce a new export without even knowing about it. We also lose the ability to export a symbol purely for internal purposes, which is useful for tests.

As it relates to the change being reverted, because of the introduction of this wildcard export, new exports have been introduced which will now need to be reverted in a new major version, namely AllowedActions and AllowedEvents. This is non-ideal; we should not have to introduce major versions if we do not need to, because they are disruptive.

Elsewhere it was noted that needing to add an export manually to this list is a small pain point especially if other subdirectories also use index.ts. I would argue that not knowing about exports (regardless of at which level exports occur) creates more trouble in the long run. That is, the small pain point is a intentional feature and not a bug :)

References

See #5224.

Changelog

(None; no functional changes)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

Using a wildcard export in the root `index.ts` of a package is not an
ideal practice. This file is very useful because it gives us a way to
understand everything that a package publicly exports at a glance.

In using a wildcard export, however, we lose this ability. Because all
symbols marked with `export` will automatically become exports of the
package, we can no see when a new package is introduced in a PR — so it
is possible to introduce a new export without even knowing about it —
and we lose the ability to export a symbol purely for internal purposes,
which is useful for tests.

As it relates to the change being reverted, because of the introduction
of this wildcard export, new exports have been introduced which will now
need to be reverted in a new major version, namely `AllowedActions` and
`AllowedEvents`. This is non-ideal; we should not have to introduce
major versions if we do not need to, because they are disruptive.

Earlier it was noted that needing to add an export manually to this list
is a small pain point. I would argue that not knowing about new exports
creates _more_ of a pain point in the long run. That is, this is a
feature and not a bug :)
@mcmire mcmire marked this pull request as ready for review February 6, 2025 23:52
@mcmire mcmire requested a review from a team as a code owner February 6, 2025 23:52
@ccharly ccharly changed the title Revert wildcard export in AccountsController refactor(accounts-controller)!: revert wildcard export Feb 7, 2025
@ccharly ccharly enabled auto-merge (squash) February 11, 2025 10:46
@ccharly ccharly merged commit 8339f49 into main Feb 11, 2025
127 checks passed
@ccharly ccharly deleted the revert-wildcard-exports-in-accounts-controller branch February 11, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants