unsplash / intlc

Compile ICU messages into code. Supports TypeScript and JSX. No runtime.
MIT License
57 stars 3 forks source link

Output with TSX missing React import #32

Closed samhh closed 2 years ago

samhh commented 2 years ago

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: https://github.com/unsplash/intlc/issues/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 commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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 ;)

https://github.com/unsplash/intlc/pull/34