Skip to content

Expand debug statements in place to permit subsequent Babel plugin translation#103

Merged
ef4 merged 3 commits intoember-cli:mainfrom
robbytx:expand-macros-in-place
Jun 24, 2025
Merged

Expand debug statements in place to permit subsequent Babel plugin translation#103
ef4 merged 3 commits intoember-cli:mainfrom
robbytx:expand-macros-in-place

Conversation

@robbytx
Copy link
Contributor

@robbytx robbytx commented Jun 13, 2025

Problem

Whenever this plugin encounters a debug function call statement, its builder performs a partial translation that gets fully resolved and substituted only once expandMacros is called at the exit of the file.

While this behavior works fine for standalone translation, the deferred substitution fails to permit other registered Babel plugins the opportunity to translate the replacement expression, due to the nature of Babel's one-shot AST traversal.

In particular, this poses a problem when using the '@embroider/macros' condition, since the resulting isDevelopingApp() call expression is not seen by the later Babel plugin from @embroider/macros.

Solution

Rather than collecting all expressions for later expansion, this change expands the debug statements in-place, and in doing so, Babel passes the replacement expression to all registered plugins. (This includes the current plugin, but the replacement expression statement is always a logical expression rather than a direct call statement, so it is not subject to re-processing by this plugin.)

Concern: Chesterton's Fence

It is unclear to me why the plugin was originally written in the manner where it collects expressions for later substitution, and whether modifying this behavior will lead to other problems. It appears that this was the behavior from the initial commit 99c4e6b, and the purpose in that code seems to have been that logic for calculating the debug flag was implemented in the Program.exit hook, so the final expression was only known at that point. It's unclear to me why exactly this approach was taken and has been preserved through a variety of refactors, but I suspect it is related in some way to the need to determine whether/how to adjust module imports, which is no longer a necessity. My hope therefore is that the effect of this change will not have adverse effects on existing usages.

@ef4
Copy link
Contributor

ef4 commented Jun 24, 2025

Thanks, this seems good to me. Out of an abundance of caution I'm going to label it at breaking so we release this as a major, so that it doesn't destabilize any of the much older apps that could be depending on the 1.x of this package. Embroider can use the 2.x directly.

@ef4 ef4 merged commit c75c201 into ember-cli:main Jun 24, 2025
6 checks passed
@github-actions github-actions bot mentioned this pull request Jun 24, 2025
@robbytx robbytx deleted the expand-macros-in-place branch June 27, 2025 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants