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

Remove FlowNodes and FlowFlags from public API #58036

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

jakebailey
Copy link
Member

These types are not referenced anywhere in our public API. #57977 modifies the flow nodes, but notes that the change is necessarily breaking.

However, these types are not even used in our public API. To be able use them, you'd already need to be using TS internals to poke at properties that were marked @internal. This PR marks them as internal, and you can see that nothing breaks in the output declaration files.

This will break https://github.com/dsherret/ts-ast-viewer/blob/74d0ba30e8bce861279a79e8f7d09634be9034d6/src/components/FlowNodeGraph.tsx at compile time, but #57977 and other changes like it will break it at runtime too. Given one already has to poke at internals, I feel like they should need internals to even have the types too...

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 2, 2024
@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

@jakebailey
Copy link
Member Author

jakebailey commented Apr 2, 2024

@jakebailey
Copy link
Member Author

jakebailey commented Apr 2, 2024

@dsherret I know that the flow node viewer was a recent and amazing addition to ts-ast-viewer; is a PR like this problematic for you? My impression is that in order to access the info, it already must reach into the internals, so it doesn't seem too far off to also need to do something special here for the types. We have other PRs in progress that will change the flow node API in a breaking way, so I suspect that the viewer will need to be updated in various ways anyway (and probably wasn't consistent in older releases either).

@dsherret
Copy link
Contributor

dsherret commented Apr 2, 2024

That's fine. I'll deal with whatever happens 😄

@jakebailey jakebailey marked this pull request as ready for review April 2, 2024 17:26
@Andarist
Copy link
Contributor

Andarist commented Apr 2, 2024

I'll deal with whatever happens 😄

Real 10x dev right here 😉

@jakebailey jakebailey merged commit 442720b into microsoft:main Apr 3, 2024
25 checks passed
@jakebailey jakebailey deleted the remove-flow-nodes branch April 3, 2024 01:24
@jakebailey jakebailey added the Breaking Change Would introduce errors in existing code label Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants