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

[draft] Upgrade TS to 4.5.x, fix some of the broken tests #192

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

karmeleon
Copy link

Hi! I'm working with a codebase that heavily uses the import { type A } from '...'; syntax that is currently broken (see issue 185). So I'm trying to update the version of TS used in flowgen to latest, stopping at 4.5.x first for simplicity's sake.

It's quite a bit more involved than I expected. Actually fixing the import type syntax was trivial, but it turns out that sometime between TS 4.4 and 4.5 there was a bugfix for the getSymbolAtLocation() function that causes it to return symbols in several situations where previously it would simply return null. You can see this easily by selecting AlbumLabel on line 3 of this TS AST viewer and changing the TS version used to generate it under Options: under 4.4 the Symbol is shown as None in the right-hand column, but it's correctly identified in 4.5 and newer. I haven't been able to figure out exactly under what circumstances the behavior changed. I looked through the git history of the TS repo but couldn't find any commits that mentioned that function at all, and the only evidence I could find of it online were this old, unresolved bug report and this SO question.

Several parts of flowgen seem to implicitly rely on this buggy behavior, and will emit incorrect Flow defs when TS returns non-null. I've been able to guess my way into "fixing" one or two instances of this, but I'm not convinced what I did was actually correct. I think some parts of flowgen might require nontrivial rewrites to work with the new behavior, but I don't know enough about flowgen to say for sure. Does anyone have any suggestions as to what I can try next?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant