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

Output with TSX missing React import #32

Closed
samhh opened this issue Jan 21, 2022 · 4 comments · Fixed by #34
Closed

Output with TSX missing React import #32

samhh opened this issue Jan 21, 2022 · 4 comments · Fixed by #34
Assignees
Labels

Comments

@samhh
Copy link
Contributor

samhh commented Jan 21, 2022

Needs ReactElement imported, and whatever best practices are now for bringing "React" and JSX sugar into scope. Let's not worry for now about other libraries/pragmas, that's tracked here: #33

We don't want to import this if we haven't used the "tsx" backend.

Going potentially further than this it's possible to use the aforementioned backend but still not require the imports, for example:

{
  "x": {
    "message": "y",
    "backend": "tsx"
  }
}
export const x: string = 'y'

A cheap and dirty way to do this would be to search the output string for ReactElement. A better way would be to use the AST to know with confidence whether or not this has happened. One way to know is, in the JavaScript compiler, if we've ever unwrapped a Lambda with a JSX InterpStrat - I think the dynamism of a lambda directly correlates to needing the import. Alternatively it may be cleaner to figure this out via the ICU Dataset as then the logic can be a single, simple predicate function that exists outside of the complexity of the compiler.

@Magellol
Copy link
Member

Magellol commented Jan 21, 2022

Going potentially further than this it's possible to use the aforementioned backend but still not require the imports, for example:

As a first version, perhaps it'll be fine to just find any possible nodes that have the tsx backend and import React regardless if we find one in the json. Even if we don't use any fragment or other react feature, I think the import won't have any impact on the runtime code. WDYT?

I'm suggesting this mainly because I have no idea what I'm doing with haskell so I'll need a simpler scope at first I think

@OliverJAsh
Copy link
Member

OliverJAsh commented Jan 24, 2022

I think import * as React from 'react'; would suffice.

Going potentially further than this it's possible to use the aforementioned backend but still not require the imports

For this case we could just include the import anyway and trust that tree shaking will remove it.

Note I think there are two reasons we need to import React:

  • so we can reference the type React.ReactElement
  • JSX compiles to React.createElement so React must be in scope

With the new JSX transform (PR), we no longer need to worry about importing React for JSX compilation. Once we've migrated to the new transform, the React import will still be necessary but only so we can reference the types.

@samhh
Copy link
Contributor Author

samhh commented Jan 24, 2022

This isn't worth extra effort to support right now, but long-term I can imagine someone wanting to use intlc with only the ts backend, not having React installed, and discovering an error due to an inapplicable import.

It'd be good to at least track support for that in another ticket. To my mind that should be solved before we potentially open source* this later in the year once the dust has settled on our web implementation.

* I think this is a good candidate for open sourcing with a blog post attached detailing how we solved internationalisation and why we opted to write our own tool.

@Magellol
Copy link
Member

I think import * as React from 'react'; would suffice.

That is the approach I took. Eager to get feedback on this PR, it's the first haskell real code I write ;)

#34

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

Successfully merging a pull request may close this issue.

3 participants