Skip to content

Conversation

@dfreeman
Copy link
Contributor

@ef4 I've been spiking out a version of ember-css-modules that uses the alpha JSUtils functionality to transform local-class attributes into import references and found that repeated calls to bindExpression and bindImport can result in erroneous or redundant code.

I've taken a pass at handling these cases more gracefully, but I'm not 100% sure of my mental model for the relationship between our locals array and what's ultimately produced as scope, so if I've got a bad assumption here I'm happy to make changes.

bindExpression

If I call bindExpression('2 + 2', target, { nameHint: 'two' }) twice in the same template, this would wind up emitting code like this, which throws a SyntaxError due to the duplicate declaration:

let two = 1 + 1;
let two = 1 + 1;

This PR updates bindExpression to ensure it accounts for locals we introduced ourselves when looking for a fresh name.

bindImport

If I call bindImport('my-library', 'default', target, { nameHint: 'two' }) twice in the same template, the code produced isn't incorrect, but it's a bit weird. In the tests, you'll see output like this, with two appearing twice in locals:

import { createTemplateFactory } from "@ember/template-factory";
import two from "my-library";
export default function () {
  const template = createTemplateFactory(
  /*
    {{onePlusOne}}{{onePlusOne}}
  */
  {
    transformedHBS: `{{two}}{{two}}`,
    locals: [two, two]
  });
}

This PR updates bindImport to reuse the existing identifier if possible, ensuring that we still alias it if necessary due to other elements in template scope.

@ef4
Copy link
Contributor

ef4 commented Aug 11, 2022

Apologies, but I've made this confusing by having two PRs going at once. I think these bugs might be fixed already in #9

@ef4
Copy link
Contributor

ef4 commented Aug 11, 2022

I will consolidate into one branch that is the 2.0 release candidate.

@dfreeman
Copy link
Contributor Author

Ah, I didn't realize further changes to JSUtils had happened in #9. Looking at the diff, I expect the registerBinding call probably fixes the emitExpression collision issue.

The duplicate locals/scope entries from bindImport were actually something I originally noticed with 2.0.0-alpha.0, though, and I don't immediately see anything there that would address that. scope: () => ({ foo, foo, foo, foo }) isn't going to blow up at runtime, but it does seem preferable to avoid the bloat if we can.

@ef4
Copy link
Contributor

ef4 commented Oct 31, 2022

Rebased and merged into jsutils.

@ef4 ef4 closed this Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants