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

Initial inline value support #45173

Closed
wants to merge 7 commits into from
Closed

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Jul 23, 2021

Fixes #43449

@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #43449. If you can get it accepted, this PR will have a better chance of being reviewed.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 23, 2021
@Kingwl Kingwl marked this pull request as draft July 23, 2021 21:05
@Kingwl
Copy link
Contributor Author

Kingwl commented Jul 23, 2021

@Kingwl
Copy link
Contributor Author

Kingwl commented Jul 28, 2021

Not pretty sure what should we do in class-like.

@Kingwl Kingwl marked this pull request as ready for review July 30, 2021 07:32
@Kingwl
Copy link
Contributor Author

Kingwl commented Jul 30, 2021

Basically ready for review except class like element.

@sandersn sandersn added this to Not started in PR Backlog Aug 2, 2021
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Aug 2, 2021
@andrewbranch
Copy link
Member

Thanks @Kingwl! FYI, we do want the implementation to go in a TS Server plugin rather than in TS Server directly. I believe we have the ability to do that thanks to your PR at #44291. I also need to understand more about what VS Code wants with the expression spans we return in order to give any implementation a good review. Maybe @connor4312 and @hediet can help here: mainly I was surprised by this part of the description from #43449:

Property assignments should be evaluated, foo() in { x: foo() }

I just want to confirm that it’s ok that the expressions in the spans we return may have side effects. Obviously a call expression can have side effects, but property expressions can too (through getters). I want to ensure that our syntactic analysis is not supposed to filter out constructs that are possibly-effectful.

@connor4312
Copy link
Member

I just want to confirm that it’s ok that the expressions in the spans we return may have side effects.

Yea, that's fine. V8 provides debugger-level functionality that lets us prevent evaluations from causing side effects.

@andrewbranch andrewbranch assigned andrewbranch and unassigned amcasey Sep 8, 2021
@Kingwl Kingwl closed this Oct 26, 2021
PR Backlog automation moved this from Waiting on reviewers to Done Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Add Inline value provider api for debugging
5 participants