Skip to content

Fix crash inside JSX Closing tag #6006

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

Merged
merged 12 commits into from
Jan 10, 2016
Merged

Fix crash inside JSX Closing tag #6006

merged 12 commits into from
Jan 10, 2016

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Dec 9, 2015

Fix issue #5953 by giving its associated opening tag. Initially, the PR tried to address preventing adding the global symbols that is not in jSX into the opening tag. However, doing so cause some inconsistent so the change is rolled back.

if (tryGetGlobalSymbols()) {
// If the currect cursor is inside JSX opening tag, the only meaningful completions are those of JSX.IntrinsicElements or users defined React.Component
// If the services can't find those symbols, then show nothing instead of including all the global symbols in the completion list.
if (tryGetGlobalSymbols(/*includeAllGlobalSymbols*/false)) {
Copy link
Member

Choose a reason for hiding this comment

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

Space after comment

Copy link
Member

Choose a reason for hiding this comment

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

What does "all" mean? Something is getting filtered out and this parameter name doesn't convey it well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on naming. not sure what is the best name. What I try to convey is that the boolen control whether you will add all global symbols from all the files into the completions list or not. This doesn't make sense for JSX opening tag

@DanielRosenwasser
Copy link
Member

After some though, I've realized that this sort of thing is potentially obstructive to the way that users might write code. For instance, let's say I have

var x =
    <div>
        <span>
    </

Let's say I'm actually simply intent on closing my div element. I'm going to try to come back to my span element later, but you're giving no suggestions to me. Now that impedes the flow as I type, expecting to be completed by div.

I think you should consider giving completions for all containing open tags who don't have an appropriate corresponding closing tag. That means that for:

<div>
  <span>
    <a>
  </ span
</ div>

It might be the case that the a is considered closed, but its matching closing tag is span. That means a probably still needs to be closed, so you should show it in the completion list. That also means that div and span also lack a matching tag (<span> gets closed with </ div>, and <div> is unclosed).

We should discuss this a bit tomorrow.

@RyanCavanaugh
Copy link
Member

Let's say I'm actually simply intent on closing my div element

This seems like an odd state to be in.

@bowdenk7 - Maybe we can get some people writing JSX in our next usability study so we can see what order people actually go in. That could be very informative.

@yuit
Copy link
Contributor Author

yuit commented Dec 18, 2015

@DanielRosenwasser yes. I don't think I got what you trying to say in the last part of the comment
and I agree with @RyanCavanaugh it is odd to come back to close the tag later. I can see more that you want to come back and type between tags.

Also to clarify, this behavior is only when you don't have react.d.ts.

@DanielRosenwasser
Copy link
Member

We only show the closing tag element when you don't have a valid JSX namespace? That seems fine then. I'm mainly trying to avoid a scenario where you have someone with extremely weird typing habits (like myself) where they end up fighting the editor.

@yuit
Copy link
Contributor Author

yuit commented Dec 18, 2015

@DanielRosenwasser yep, this behavior is only trigger when we don't have valid JSX name.

Conflicts:
	src/services/services.ts
@yuit
Copy link
Contributor Author

yuit commented Dec 20, 2015

@DanielRosenwasser @RyanCavanaugh @sandersn Any more comments?

@DanielRosenwasser
Copy link
Member

We're exposing this in our API, so we should discuss this with @mhegazy in a meeting, but other than that 👍

@mhegazy
Copy link
Contributor

mhegazy commented Jan 7, 2016

👍

yuit added a commit that referenced this pull request Jan 10, 2016
@yuit yuit merged commit 8cf347c into master Jan 10, 2016
@yuit yuit deleted the fix5953_crashJSX branch January 10, 2016 14:23
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants