embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
333 stars 136 forks source link

@babel/template placeholder build error resurrection #1077

Closed Windvis closed 2 years ago

Windvis commented 2 years ago

Since the release of ember-get-config v1 (which uses @embroider/macros v0.50.0) several people in the Discord are seeing the following errors at build time:

broccoliBuilderErrorStack: Error: @babel/template placeholder "APP": Expected string substitution

This seems to be the issue that was fixed by this commit and was included in the 0.43.0 release of @embroider/macros.

In our app the issue is triggered by @embroider/macros v0.41.0 which is included by the ember-element-helper addon. What's strange is that that version of the macros didn't cause issues until a v0.50.0 release was part of our node_modules folder.

I'm unfamiliar with how this works so my debugging efforts didn't lead to something useful. Did anything change in the latest release that might explain this behavior? It probably worked by accident before and we should PR version bumps of the macro packages to all addons?

npm ls @embroider/macros of our project:

├─┬ ember-auto-import@1.12.0
│ └─┬ @embroider/core@0.33.0
│   └── @embroider/macros@0.33.0 
├─┬ ember-cli-moment-shim@3.8.0
│ └─┬ ember-get-config@1.0.0
│   └── @embroider/macros@0.50.1 
└─┬ ember-power-select@4.1.7
  └─┬ ember-basic-dropdown@3.1.0
    ├─┬ @ember/render-modifiers@2.0.2
    │ └── @embroider/macros@0.50.1 
    ├── @embroider/macros@0.47.2 
    ├─┬ @embroider/util@0.47.2
    │ └── @embroider/macros@0.47.2 
    └─┬ ember-element-helper@0.5.5
      └─┬ @embroider/util@0.41.0
        └── @embroider/macros@0.41.0 

Discord discussion: https://discord.com/channels/480462759797063690/930822209725948004

obrunofontana commented 2 years ago

I have the same problem, i corrected it by fixing the lib version ember-cli-moment-shim": "3.7.1", however, just a quick solution!

jrjohnson commented 2 years ago

We're seeing this in an addon as @babel/template placeholder "LOG_VIEW_LOOKUPS": Expected string substitution

navels commented 2 years ago

Downgrading ember-cli-moment-shim isn't an option for us as it breaks our production build (https://github.com/terser/terser/issues/684#ref-issue-613919275).

navels commented 2 years ago

This seems to be fixed in 0.50.1 and adding this resolution to our package.json has fixed our build until ember-get-config can update:

  "resolutions": {
    "**/@embroider/macros": "0.50.1"
  }
navels commented 2 years ago

PR: https://github.com/mansona/ember-get-config/pull/32

RobbieTheWagner commented 2 years ago

@navels I have been talking with @mansona and we did some digging. The real issue here is if you have any addons using < embroider 0.50 and then any addon using > 0.50 things go 💥. The best fix I have found for now, is to do:

    "**/@embroider/macros": "< 0.50.0",
    "**/@embroider/util": "< 0.50.0",
jrjohnson commented 2 years ago

any addons using < embroider 0.50 and then any addon using > 0.50 things go 💥

That's gonnaa be tricky since the latest ember-cli LTS includes ember-auto-import@1.12.0 which requires @embroider/core@0.33.0. So we have quite a few addons that fall into this version gap.

RobbieTheWagner commented 2 years ago

Yes, it is quite a bad problem.

@ef4 any plans on a fix so the versions can play nicely together?

ef4 commented 2 years ago

There were no significant changes to the macro system in 0.50.0. Can somebody share the contents of the value that's being passed to babelContext.template that makes it blow up?

It is true that all copies of the macros share the state that is being inlined into the source at this spot, so additional versions being around can influence the value here.

jrjohnson commented 2 years ago

I'm not sure how to do that @ef4, I looked through the trace and I'm not sure where to put a debugger to get that info.

ef4 commented 2 years ago

Find this spot in the @embroider/macros package in your node_modules:

https://github.com/embroider-build/embroider/blob/f31f32bf9ae3b3c50f7c5f59525d886602e90663/packages/macros/src/babel/evaluate-json.ts#L428

And console.log(JSON.stringify(value))

jrjohnson commented 2 years ago

Looks a bit different, but I changed evaluate-json.js in our addon to

function buildLiterals(value, babelContext) {
    if (typeof value === 'undefined') {
        return babelContext.types.identifier('undefined');
    }
    console.log("\n\nOUTPUT:\n", JSON.stringify(value), "\n\n");
    let statement = babelContext.parse(`a(${JSON.stringify(value)})`);
    let expression = statement.program.body[0].expression;
    return expression.arguments[0];
}

and the output looks like:

OUTPUT:
 {"packages":{"APP_HOME/node_modules/ember-cli-moment-shim/node_modules/ember-get-config":{"config":{"modulePrefix":"dummy","environment":"development","rootURL":"/","locationType":"auto","intl":{"defaultLocale":"en"}},"EmberENV":{"FEATURES":{},"EXTEND_PROTOTYPES":{"Date":false},"DEBUG_TASKS":true,"_APPLICATION_TEMPLATE_WRAPPER":false,"_DEFAULT_ASYNC_OBSERVERS":true,"_JQUERY_INTEGRATION":false,"_TEMPLATE_ONLY_GLIMMER_COMPONENTS":true},"APP":{},"ember-cli-mirage":{"enabled":false,"usingProxy":false,"useDefaultPassthroughs":true},"moment":{"includeLocales":["es","fr"],"includeTimezone":"all"},"featureFlags":{"sessionLinkingAdminUi":true},"apiVersion":"v3.4","exportApplicationGlobal":true}}},"global":{"@embroider/macros":{"isTesting":false}}} 

OUTPUT:
 {"packages":{"APP_HOME/node_modules/ember-cli-moment-shim/node_modules/ember-get-config":{"config":{"modulePrefix":"dummy","environment":"development","rootURL":"/","locationType":"auto","intl":{"defaultLocale":"en"}},"EmberENV":{"FEATURES":{},"EXTEND_PROTOTYPES":{"Date":false},"DEBUG_TASKS":true,"_APPLICATION_TEMPLATE_WRAPPER":false,"_DEFAULT_ASYNC_OBSERVERS":true,"_JQUERY_INTEGRATION":false,"_TEMPLATE_ONLY_GLIMMER_COMPONENTS":true},"APP":{},"ember-cli-mirage":{"enabled":false,"usingProxy":false,"useDefaultPassthroughs":true},"moment":{"includeLocales":["es","fr"],"includeTimezone":"all"},"featureFlags":{"sessionLinkingAdminUi":true},"apiVersion":"v3.4","exportApplicationGlobal":true}}},"global":{"@embroider/macros":{"isTesting":false}}} 

OUTPUT:
 {"packages":{"APP_HOME/node_modules/ember-cli-moment-shim/node_modules/ember-get-config":{"config":{"modulePrefix":"dummy","environment":"development","rootURL":"/","locationType":"auto","intl":{"defaultLocale":"en"}},"EmberENV":{"FEATURES":{},"EXTEND_PROTOTYPES":{"Date":false},"DEBUG_TASKS":true,"_APPLICATION_TEMPLATE_WRAPPER":false,"_DEFAULT_ASYNC_OBSERVERS":true,"_JQUERY_INTEGRATION":false,"_TEMPLATE_ONLY_GLIMMER_COMPONENTS":true},"APP":{},"ember-cli-mirage":{"enabled":false,"usingProxy":false,"useDefaultPassthroughs":true},"moment":{"includeLocales":["es","fr"],"includeTimezone":"all"},"featureFlags":{"sessionLinkingAdminUi":true},"apiVersion":"v3.4","exportApplicationGlobal":true}}},"global":{"@embroider/macros":{"isTesting":false}}} 

⠹ building... [Babel: @embroider/macros > applyPatches]

OUTPUT:
 true 

OUTPUT:
 true 

OUTPUT:
 true 

OUTPUT:
 true 

OUTPUT:
 true 

OUTPUT:
 true 

OUTPUT:
 true 

Build Error (broccoli-persistent-filter:Babel > [Babel: @embroider/macros]) in @embroider/macros/runtime.js

APP_HOME/@embroider/macros/runtime.js: @babel/template placeholder "APP": Expected string substitution
navels commented 2 years ago

Basically it is the package's config/environment.js that is getting parsed incorrectly.

Windvis commented 2 years ago

It seems it's very easy to reproduce:

  1. Create a new Ember app (I used 3.28)
  2. Install ember-element-helper (it uses @embroider/util 0.41.0). The build will still work fine after this.
  3. Install ember-get-config v1. The build will fail after installing this package.

Reproduction app: https://github.com/Windvis/embroider-macros-babel-template-error-reproduction (Removed since the issue is resolved.)

I also tried installing @embroider/macros v0.50.0 as a direct dependency, but that doesn't make the build fail. So it seems to be related to ember-get-config's usage of the macros.

ef4 commented 2 years ago

So it seems to be related to ember-get-config's usage of the macros.

Ah, this is the actual issue. The format of the data that ember-get-config is storing in the macros is what triggers the bug. It's not really about version skew at all.

We could backport the fix and do patch releases, but we'd potentially need to manage 17 different patch releases, because the oldest embroider/macros mentioned here is 17 major releases behind.

I think the biggest thing we can do to help here is to cut a 1.0 release, so that the pain of asking addons to upgrade can happen once and then they'll be on a series that we can actually support with patch releases going forward.

I had been waiting to release a 1.0 until after solving some known limitations, but we're past the point where it's practical to consider this stuff prerelease.

ef4 commented 2 years ago

Ok, so 1.0 is released. Please spread the word, addons should depend on "@embroider/macros": "^1.0.0". There are no breaking changes in 1.0.

In the meantime, both yarn and npm now support overriding transitive dependencies. In the case of @embroider/macros it is safe to replace all of 0.x with ^1.0.0.

NPM added overrides in 8.3. Yarn has resolutions.

Windvis commented 2 years ago

Since it wasn't an actual bug I think we can close this now. Thanks for your insights and the new release!

navels commented 2 years ago

thanks!