stylex icon indicating copy to clipboard operation
stylex copied to clipboard

Safe typings for TypeScript v4

Open dimitur2204 opened this issue 1 year ago • 12 comments

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

dimitur2204 avatar Jan 09 '24 13:01 dimitur2204

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+.

nmn avatar Jan 09 '24 18:01 nmn

I can handle this issue if possible

amjadorfali avatar Jan 09 '24 18:01 amjadorfali

@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.

nmn avatar Jan 10 '24 08:01 nmn

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

  • What should we write?
  • Where should we write it? (Any tips)

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

amjadorfali avatar Jan 10 '24 09:01 amjadorfali

@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?

nmn avatar Jan 10 '24 10:01 nmn

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

amjadorfali avatar Jan 10 '24 17:01 amjadorfali

Let's start with the API documentation page.

nmn avatar Jan 10 '24 19:01 nmn

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

pawelblaszczyk5 avatar Jan 11 '24 08:01 pawelblaszczyk5

This looks doable. I'll look into it.

nmn avatar Jan 11 '24 11:01 nmn

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.

dimitur2204 avatar Jan 11 '24 11:01 dimitur2204

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.

nmn avatar Jan 11 '24 12:01 nmn

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.

dimitur2204 avatar Jan 11 '24 12:01 dimitur2204