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

Fix branch elimination for macroDependencySatisfies #1688

Merged
merged 4 commits into from
Dec 13, 2023

Conversation

mike-engel
Copy link
Contributor

This is a follow up to #1468, and should fix ember-animation/ember-animated#647.

The problem is that #1468 only fixed branch elimination for non-runtime macros in JS/TS code via babel, but didn't fix it for templates via glimmer. This fixes macroDependencySatisfies by transforming it into a macroCondition in the first pass, which will eliminate the branch in the second pass when it's used as a sub-expression.

Writing code like macroCondition (macroDependencySatisfies 'qunit' '^2.8.0') will work, but is not ergonomic and is unfriendly.

I don't think this needs to be done for macroGetConfig or macroGetOwnConfig.

@NullVoxPopuli
Copy link
Collaborator

thanks for submitting this! looks like you have some test failures there!

@mike-engel
Copy link
Contributor Author

Whoops, I thought they were all passing before I pushed.

@NullVoxPopuli I added an extra case to handle macro compositions, but it's starting to feel a little hacky. An alternative to this PR would be to expand the docs to call out explicitly that macroDependencySatisfies requires a parent macroCondition in templates to properly prune branches. That also feels a little hacky, but it's potentially less brittle.

@mansona mansona changed the base branch from main to stable December 13, 2023 15:41
@mansona
Copy link
Member

mansona commented Dec 13, 2023

Hey tests were failing on strange type problems so I rebased this 👍 I also re-targeted it to stable so we can get the bugfix out the door

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

Looks great 🎉

@mansona mansona merged commit 8b8f731 into embroider-build:stable Dec 13, 2023
201 checks passed
@github-actions github-actions bot mentioned this pull request Dec 13, 2023
@mike-engel mike-engel deleted the glimmer-branch-elimination branch December 14, 2023 09:02
@mike-engel
Copy link
Contributor Author

Wow, that was simple! Thanks @mansona

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants