facebook / stylex

StyleX is the styling system for ambitious user interfaces.
https://stylexjs.com
MIT License
8.36k stars 307 forks source link

Safe typings for TypeScript v4 #327

Open dimitur2204 opened 9 months ago

dimitur2204 commented 9 months ago

Is your feature request related to a problem? Please describe. The problem is the one mentioned in this issue https://github.com/facebook/stylex/issues/313, where the TypeScript const modifier is used (https://github.com/microsoft/TypeScript/pull/51865) which was introduced in v5 from what I can see.

Describe a solution you'd like Some way to make the types safe for earlier versions of TypeScript

Describe alternatives you've considered If that is not possible, than atleast some kind of warning somewhere in the docs should be present to indicate the problem

Additional context

image

nmn commented 9 months ago

I'm not aware of a way to offer alternate types for older versions of typescript. Unless that is possible, we will just document the dependency on Typescript v5+.

amjadorfali commented 9 months ago

I can handle this issue if possible

nmn commented 9 months ago

@amjadorfali We might need to iterate a bit to find the best page to document this requirement, but if you're willing to go through that process, I'm happy to review.

amjadorfali commented 9 months ago

@nmn I'd be happy to take over this issue. Can you please provide input on the following

I'm new to open source so i'm not sure if i should be answering these questions myself or the core maintainers should.

nmn commented 9 months ago

@amjadorfali As this is a documentation task, you should read through the documentation and propose answers to those questions yourself.

What should we write?

This is easy to answer. You can probably add an admonition (Wrapped in :::warning ... :::, look for existing examples) that says something simple like "requires Typescript 5 or newer".

Where?

I'm not sure. Definitely not the Thinking in StyleX page. Perhaps the Learn/Static types page?

amjadorfali commented 9 months ago

I want to take your opinion before implementation. As you mentioned we can write it in Learn/Static types; maybe instead we should write it under the API documentation for this API?

An alternative would be to add a header in Learn/Static types specific for Limitations

Please let me know what you think @nmn

P.S: There are many other pages that reference stylex.create

nmn commented 9 months ago

Let's start with the API documentation page.

pawelblaszczyk5 commented 9 months ago

There is an option to provide alternate types based on TypeScript version - https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#version-selection-with-typesversions

nmn commented 9 months ago

This looks doable. I'll look into it.

dimitur2204 commented 9 months ago

There is an option to provide alternate types based on TypeScript version - https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#version-selection-with-typesversions

Would the same typing be possible without that const declaration though? If it isn't, then is the team at stylex okay with providing different typings for different TypeScript versions. Maybe there is a concern with consistency in that case. On the other hand everything is better than the current behaviour.

nmn commented 9 months ago

Would the same typing be possible without that const declaration though?

It would be, but you'd need to manually add a as const after each style object.

Could you explain why you can't update Typescript for your project. As I understand, Typescript 5 is more than a year old.

dimitur2204 commented 9 months ago

Could you explain why you can't update Typescript for your project. As I understand, Typescript 5 is more than a year old.

I have a multiple repo system where I am thinking of migrating to stylex from various different styling solutions across them currently. All of the repos however use Typescript 4.7.3. It's not that I can't update, it's just that it adds some traction.

At the end of the day the decision is on the team here if that fix is worth the time. If it isn't then the addition to the docs will at least warn people.