ef4 / decorator-transforms

Better babel transforms for decorators
8 stars 5 forks source link

Mutate existing default export declarations for improved compatibility #18

Closed davidtaylorhq closed 8 months ago

davidtaylorhq commented 8 months ago

Calling t.exportDefaultDeclaration triggers the ExportDefaultDeclaration in other babel transforms. In some cases triggering that hook multiple times can lead to unexpected behavior.

For example, in the the template-colocation-plugin at https://github.com/embroider-build/embroider/blob/289f83d80d/packages/shared-internals/src/template-colocation-plugin.ts), the first call of the hook will see a class declaration and set state.associate^1 (which later causes setComponentTemplate to be appended after the export ^2). The second call of the hook sees a non-class expression and wraps it in setComponentTemplate within the export ^3. In the end, that leaves us with two setComponentTemplate calls.


Resolves #16

This issue could alternatively be fixed with a change to the template-colocation-plugin itself (e.g. have it unset state.associate on the second invocation of ExportDefaultDeclaration.

I'm unsure how to go about adding a test for this. Do we want to try and pull the template-colocation-plugin into the decorator-transforms test suite somehow? 🤔

ef4 commented 8 months ago

I added tests here: https://github.com/ef4/decorator-transforms/pull/19

I agree with your diagnosis but I think we should fix in @embroider/shared-internals instead of here. That is the plugin that cares about idempotence, so it should enforce its own. Notably, the very similar colocation plugin in ember-cli-htmlbars does not suffer this bug because it correctly knows not to apply colocation twice to one file.

We can't guarantee that no other plugin out there is going to rewrite a class using babel mutation-aware APIs, so even if we stopped using them in this plugin we would still need to change the one in shared-internals.

ef4 commented 8 months ago

https://github.com/embroider-build/embroider/pull/1758