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

Explicitly import anchored-region in menu with side-effects #1152

Merged
merged 3 commits into from
Apr 3, 2023

Conversation

rajsite
Copy link
Member

@rajsite rajsite commented Apr 3, 2023

Pull Request

🀨 Rationale

πŸ‘©β€πŸ’» Implementation

As part of #1134 the menu element definition added an import to the anchored region as an unused variable for the element registration side-effect.

Build tooling with dead code elimination may try to strip out that dependency under the assumption that imports do not have side effects so they can safely remove an unused variable and the associated import tree. Switched to the side-effect only import syntax which is a stronger declaration to bundling tools that the import has side-effects.

πŸ§ͺ Testing

Relying on CI, no additional tests added.

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@rajsite rajsite requested a review from jattasNI as a code owner April 3, 2023 16:51
@rajsite rajsite enabled auto-merge (squash) April 3, 2023 16:52
@rajsite rajsite disabled auto-merge April 3, 2023 16:53
@rajsite rajsite enabled auto-merge (squash) April 3, 2023 16:53
Copy link
Contributor

@jattasNI jattasNI 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 catching and fixing this!

@rajsite rajsite merged commit ed97369 into main Apr 3, 2023
@rajsite rajsite deleted the explicit-side-effect-import branch April 3, 2023 18:07
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