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

$capture_state breaks with Babel macros #5104

Closed
rixo opened this issue Jul 5, 2020 · 2 comments
Closed

$capture_state breaks with Babel macros #5104

rixo opened this issue Jul 5, 2020 · 2 comments
Labels
bug compiler Changes relating to the compiler temp-stale

Comments

@rixo
Copy link
Contributor

rixo commented Jul 5, 2020

Describe the bug
The modifications that have been made to $capture_state a while ago make it break when the user's code contains Babel macros. That is, an import that is then transformed by some tooling and the import binding (variable) is removed from code.

We've had someone report that with Babel macros and, more recently, with Tailwind macros (also a Babel macro).

For example, in this user code, the tw binding gets removed from the code by Babel:

<script>
  import tw from "twin.macro"; // <= removed
  const bigText = tw`text-xl`; // <= transformed into: const bigText = { "fontSize": "1.25rem" };
</script>

But the compiler has seen it and it is added to $capture_state:

  $$self.$capture_state = () => ({
    tw,
    bigText,
  });

Since the variable tw doesn't exist, it crashes at runtime.

To Reproduce
The aforementioned issue has provided a minimal repo that reproduces the issue.

Expected behavior
Apparently, the ideal solution would be to exclude import bindings from $capture_state (see discussion in the previous issue).

But if I haven't missed anything, we apparently can't tell from $capture_state which variables are imports?

I don't think it's worth adding tracking of this information (if it's not already there) just for this feature. So I suppose we should settle on excluding either hoistable (I guess all imports have to be hoistable, right?) or writable variables.

For HMR it won't make a difference which one we pick. I suppose hoistable is preferable if it works as a fix, since previous discussion has concluded that it was desirable to capture writable state.

I can make a PR but I'd like to confirm what needs to be done.

cc @RedHatter @Conduitry

Severity
Tailwind seems to be pretty popular in Svelte projects, so I guess it has the potential to frustrate quite a few people.

@itsMapleLeaf
Copy link

itsMapleLeaf commented Jul 5, 2020

Thanks for writing up this issue.

Tailwind seems to be pretty popular in Svelte projects, so I guess it has the potential to frustrate quite a few people.

This is an issue specific to twin.macro and other macros. From what I've seen, most tailwind users use PostCSS and write class names directly. I like twin.macro for the static guarantees that it gives me + some additional DX bonuses, but its use isn't that prolific

Would still love to see a solution to this, though 😅

@pngwn pngwn added compiler Changes relating to the compiler temp-stale and removed feature: compiler labels Jun 27, 2021
@dummdidumm
Copy link
Member

Closing as inactive/outdated

@dummdidumm dummdidumm closed this as not planned Won't fix, can't repro, duplicate, stale Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug compiler Changes relating to the compiler temp-stale
Projects
None yet
Development

No branches or pull requests

5 participants