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

Handle already hoisted bindings #1069

Merged
merged 2 commits into from Sep 27, 2022

Conversation

paddyobrien
Copy link
Contributor

Motivation

Fixes #1047

Summary

This incorrectly failed with an "unsupported error" in cases where the identifier was already hoisted.

A couple of lines down from here we return early if the binding.scope.parent is falsey. So I think it's safe to let this through.

Test plan

I wasn't able to get the test suite (or bootstrap) to run with pnpm so I haven't included any tests.

It seems like it would be fairly easy to add a snapshot test along the lines of the following to the test file here:

  it('should work with already hoisted vars', async () => {
    const { code, metadata } = await transform(
      dedent`
        function color() {
          return 'red\
        }

        export const square = css\`
          color: ${'${color()}'}
      \`;
      `,
      [evaluator]
    );

    expect(code).toMatchSnapshot();
    expect(metadata).toMatchSnapshot();
  });

Copy link
Collaborator

@Anber Anber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @paddyobrien!
Thank you for this fix!

@paddyobrien
Copy link
Contributor Author

No problem @Anber do you think it will be merged soon? We are currently blocked on upgrading to the 4.x releases due to this issue. (And I suspect we might have other issues once we get this one fixed)

@Anber
Copy link
Collaborator

Anber commented Sep 27, 2022

@paddyobrien I'm going to release the next version in a couple of days.

@Anber Anber merged commit 24b4a4b into callstack:master Sep 27, 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.

The function cannot be used in a string template
2 participants