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

Invalid code output when using @embroider/macros (ember app) #1812

Open
nulle opened this issue Feb 22, 2024 · 2 comments
Open

Invalid code output when using @embroider/macros (ember app) #1812

nulle opened this issue Feb 22, 2024 · 2 comments

Comments

@nulle
Copy link

nulle commented Feb 22, 2024

I have an issue of @embroider/macros installation messing up the output of my Ember app code.
From what I'm guessing, it's got something to do with (incorrect) optional chaining operator implementation, and appears only in production builds.

Reproduce steps:

  1. Create a fresh EmberJS app
  2. Add the following code somewhere
const test = {
    items: [],
};
[1, 2].forEach((i) => {
    test.items.push(i);
});
return String(test.items?.[0]);
  1. ember serve --environment=production

  2. The following code outputs "1". <-- this is the expected output (first array item)

  3. install embroider/macros package
    npm install @embroider/macros --save-dev

  4. ember serve --environment=production

  5. The same code outputs "undefined" <-- not good

Not only the result is undefined, but if you look at the assets file, it's actually compiled as such: String(void 0)

There is also a minimum viable reproduce here: nulle/testapp#1

Note: the only reason I'm adding @embroider/macros is for ember-data randomUUID polyfill.

@NullVoxPopuli
Copy link
Collaborator

I was looking at the compiled code for your snippet, and it looks like the stuff inside String is converted to void

class r extends t.default {
  get test() {
    const e = { items: [] };
    return (
      [1, 2].forEach((t) => {
        e.items.push(t);
      }),
      String(void 0)
    );
  }
}

If I change the input code to:

    return `${test.items?.[0]}`;

The output is now:

class r extends t.default {
  get test() {
    const e = { items: [] };
    return (
      [1, 2].forEach((t) => {
        e.items.push(t);
      }),
      'undefined'
    );
  }
}

I had hypothesized that this could be related to the test word used in the input code, but after changing the input code to

import Controller from '@ember/controller';

export default class ApplicationController extends Controller {
  get foo() {
    const bar = {
      items: [],
    };
    [1, 2].forEach((i) => {
      bar.items.push(i);
    });
    return String(bar.items?.[0]);
  }
}

the production output is the same as the first (void)

next hypothesis is that this is actually a terser issue (maybe it's optimizing something that I can't see?) -- so with the reproduction branch, I've uninstalled @embroider/macros... but what we expect to exist in the production output is there:

class r extends t.default {
  get foo() {
    const e = { items: [] };
    return (
      [1, 2].forEach((t) => {
        e.items.push(t);
      }),
      String(e.items?.[0])
    );
  }
}

so that basically confirms this is an issue with the babel plugins that @embroider/macros is bringing in. (and this is re-confirmed after I added @embroider/macros back to this project).

If we're thinking that it's macros' babel plugin causing this behavior, a place to debug would be here:
https://github.com/embroider-build/embroider/blob/main/packages/macros/src/ember-addon-main.ts#L76

and here
https://github.com/embroider-build/embroider/blob/main/packages/macros/src/babel/macros-babel-plugin.ts#L15

And if you wanted to submit a failing test PR, you could add your scenario here: https://github.com/embroider-build/embroider/tree/main/packages/macros/tests/babel

@CvX
Copy link

CvX commented Apr 30, 2024

We hit this bug in https://github.com/discourse/discourse too. The repro can be reduced to:

let x = [];
x?.push();

(or even code-golfed to []?.at() 😉)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants