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

CLI: Do not use modern TS assets in legacy TS projects #20458

Merged
merged 9 commits into from
Feb 13, 2023
Merged

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Dec 30, 2022

Issue:

New TypeScript (4.9+) syntax components could be added to a project using an older version of TS, if the renderer did not include a ts-legacy folder.

What I did

Removed that unsafe fallback, and added some tests.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @kasperpeulen can you please take a look?

@kasperpeulen
Copy link
Contributor

@shilman @IanVS I think this will break angular and web components, as they have only a ts directory, as there is no special syntax for TS4.9 for those renderers. I didnt want to duplicate directories there.

I think we may just have to be careful to always have a ts-legacy directory when there is a ts directory with new ts features.

@IanVS
Copy link
Member Author

IanVS commented Jan 2, 2023

In that situation wouldn't it be better to name the folder ts-legacy? Then it would still be used by both, without the danger of breaking if someone creates just a new ts directory like almost just happened for Vue?

@kasperpeulen
Copy link
Contributor

In that situation wouldn't it be better to name the folder ts-legacy? Then it would still be used by both, without the danger of breaking if someone creates just a new ts directory like almost just happened for Vue?

Yeah, I understand that point, but there may never come a ts directory for angular, as I think it might be imposible to ever take adventage of satisfies there. So it would be also a bit weird to only having ts-legacy in the renderer, while we use it also for modern ts. 🤔

@IanVS
Copy link
Member Author

IanVS commented Jan 3, 2023

Thanks for explaining. I personally still think it's better to be safe and have an awkward folder name for Angular, or use a different word than legacy if that's the issue. Do we have any tests that will detect if 4.9 syntax is used in a ts folder, without a ts-legacy? This will break for users, which IMHO we should be careful to avoid.

@kasperpeulen
Copy link
Contributor

@IanVS Hmm, I would say the correct approach would be to never have "only" have a ts directory if it contains TS>=4.9 code. In that scenario, you always want to have also a ts-legacy directory, otherwise we will fallback to js in a ts project.

I guess the approach of this PR is more safe, but not sure if it is more "right" :D

Say someone makes a PR that would add a ts directory to a renderer, but not taking advantage of TS 4.9 features, when we merge such a PR, it would only install use the ts directory for TS >= 4.9 projects, and fall back to JS in TS<4.9 projects, which is not what someone would expect I guess.

If we force such the ts-legacy directory name for renderers such angular and web-components, it may look weird, and people might think, hey the ts directory is missing, while that directory would not make sense.

Anyway, I understand your point as well. What do you think @shilman ?

@shilman
Copy link
Member

shilman commented Jan 3, 2023

@kasperpeulen @IanVS I agree with both of you. I think the ideal approach would be to have the minimum version of TS that we require embedded in the directory name. So we could rename existing ts-legacy to ts-3-0 (or whatever "legacy" is currently an alias for) and the ts to ts-4-9. That way if, for some reason a user has a "TS 2.9" project it would fall back to JS (and perhaps yell at the user with the required explanation), which is weird but perhaps the right thing to do. WDYT?

@IanVS
Copy link
Member Author

IanVS commented Jan 3, 2023

Yep, I like that approach.

@IanVS IanVS marked this pull request as draft January 9, 2023 12:59
@IanVS
Copy link
Member Author

IanVS commented Jan 9, 2023

Marking as draft until I make the suggested change.

@IanVS IanVS marked this pull request as ready for review January 26, 2023 01:03
@IanVS IanVS assigned kasperpeulen and unassigned IanVS Jan 26, 2023
@IanVS IanVS requested a review from shilman January 26, 2023 01:11
@IanVS
Copy link
Member Author

IanVS commented Jan 26, 2023

The minimum TS version for our current examples is 3.8, because we use import type.

@IanVS
Copy link
Member Author

IanVS commented Feb 3, 2023

@kasperpeulen what do you think of this now?

Copy link
Contributor

@kasperpeulen kasperpeulen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

# Conflicts:
#	code/frameworks/nextjs/template/cli/ts-3-8/Introduction.mdx
#	code/frameworks/nextjs/template/cli/ts-3-8/Introduction.stories.mdx
#	code/frameworks/nextjs/template/cli/ts-legacy/Introduction.stories.mdx
@shilman shilman merged commit 3288a19 into next Feb 13, 2023
@shilman shilman deleted the typescript-assets branch February 13, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants