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

Safe typings for TypeScript v4 #327

Open
dimitur2204 opened this issue Jan 9, 2024 · 12 comments
Open

Safe typings for TypeScript v4 #327

dimitur2204 opened this issue Jan 9, 2024 · 12 comments
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@dimitur2204
Copy link

Is your feature request related to a problem? Please describe.
The problem is the one mentioned in this issue #313, where the TypeScript const modifier is used (microsoft/TypeScript#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
Copy link
Contributor

nmn commented Jan 9, 2024

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 nmn added documentation Improvements or additions to documentation good first issue Good for newcomers labels Jan 9, 2024
@amjadorfali
Copy link

I can handle this issue if possible

@nmn
Copy link
Contributor

nmn commented Jan 10, 2024

@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
Copy link

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

@nmn
Copy link
Contributor

nmn commented Jan 10, 2024

@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
Copy link

amjadorfali commented Jan 10, 2024

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
Copy link
Contributor

nmn commented Jan 10, 2024

Let's start with the API documentation page.

@pawelblaszczyk5
Copy link

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
Copy link
Contributor

nmn commented Jan 11, 2024

This looks doable. I'll look into it.

@dimitur2204
Copy link
Author

dimitur2204 commented Jan 11, 2024

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
Copy link
Contributor

nmn commented Jan 11, 2024

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
Copy link
Author

dimitur2204 commented Jan 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants