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

Use JSX highlighting and indentation added in Emacs 27 #523

Closed
wants to merge 12 commits into from

Conversation

jacksonrayhamilton
Copy link
Contributor

@jacksonrayhamilton jacksonrayhamilton commented Apr 9, 2019

The master branch of Emacs now provides support for highlighting JSX, greatly improves the current JSX indentation support, and automatically detects the use of JSX (without needing to enter js-jsx-mode). These changes allow js2-mode to tap into these improvements when using Emacs on the master branch, or eventually in version 27.

Fixes #140, #330, #389, #411, #451, #459, #462, #482, #490

I was hoping this would address #409, but I think we may have a little more work to do for that. Indentation is right in js-mode, but I think js2-mode is clearing the syntax-table text property needed to prevent JSXText from introducing unterminated strings. To solve this I will probably provide more changes (here or in another PR) that re-adds the syntax-table property here after it is cleared elsewhere.

As noted below, this now also fixes #409.

This should be a much safer way of silencing compiler warnings (I was
encountering warnings, not errors).
Since all uses of these vars and functions are now guarded by “bound” checks,
these are no longer necessary; the compiler seems to determine that proper
“bound” guards will make guarded code safe.
@jacksonrayhamilton
Copy link
Contributor Author

Since I first opened the PR last night, I got some feedback via email on the code style, and made updates accordingly (a59c7b1...63b7a72).

@jacksonrayhamilton
Copy link
Contributor Author

I pushed a commit to Emacs master tonight (“Add extra text property to fix issue with js2-mode integration”) and I pushed a complimentary commit to this PR to fix the issue with string quotes from #409.

@dgutov
Copy link
Collaborator

dgutov commented Apr 11, 2019

Hmm.

Have you tried using char-property-alias-alist instead? To create an alias for syntax-table, and either use it for JSX syntax properties, or in js2-mode everywhere instead of syntax-table?

@jacksonrayhamilton
Copy link
Contributor Author

Looks like that works! Much simpler.

While I just pushed that change, I don’t mean to discount the alternative change that Stef has suggested via email. This push is merely an improvement upon what I initially pushed. Not sure what you think about his idea, or how to test it.

@jacksonrayhamilton
Copy link
Contributor Author

I added a section to the README which should hopefully encourage some users to download a Emacs 27 snapshot and test out my improvements.

dgutov added a commit that referenced this pull request May 25, 2019
@dgutov
Copy link
Collaborator

dgutov commented May 25, 2019

After a long-ish private discussion, this is the step I have decided on for now: recommend js-mode together with js2-minor-mode as the preferred alternative. And Emacs 27, of course.

This proposal might be better in a sense (makes it easier for the users to follow), but it's harder for me to test any breakage with earlier versions of js-mode (especially going back to Emacs 24, which we still support), and the principle of not setting the syntax-table property inside the parser would introduce (probably) subtle breakage in rjsx-mode. So I'm going to hold off on it.

In the meantime, I'd like to know what @felipeochoa thinks about it and whether he'd like to reimplement some of the features in rjsx-mode in a different way, or partially merge/reuse some of the indentation code.

@jacksonrayhamilton, further comments welcome.

@dgutov
Copy link
Collaborator

dgutov commented May 25, 2019

Here's hoping js2-refactor and friends don't rely on js2-mode being used exactly. They shouldn't anyway.

@jacksonrayhamilton
Copy link
Contributor Author

Background

js-mode is now using syntax-propertize to set text properties that will allow for highlighting and indenting JSX code. Since JSX is a non-standard syntax extension, we want to enable support for it cautiously, but also easily; so, js-mode is auto-detecting the presence of JSX and enabling parsing and handling of it only under certains conditions, such as:

  • file name ends in “.jsx”
  • if import React from 'react' or var React = require('react') appears near the beginning of a buffer

This PR would have copied some code from js-mode into js2-mode, such that js2-mode would also do those things in the same way that js-mode did them. But, long term, there may be a better solution for js2-mode than copying code.

Currently, js2-mode uses js-mode’s code for indentation (and, in older emacsen, it backports indentation logic from Emacs 26 and earlier). However, js2-mode doesn’t set text properties with syntax-propertize, nor does it highlight code with font-lock-keywords. Instead, js2-mode parses the buffer and sets text properties for indenting and highlighting according to its parse. Indentation and highlighting can be more accurate as a result, although there may also be a short delay upon initial load and after changes to the buffer before those accuracies are realized by the user. In any case, for the time being and for the sake of consistency, it may make the most sense to keep things this way in js2-mode.

A disadvantage to the scheme described in the above paragraph is that the JSX features added in js-mode won’t be accessible when js2-mode is enabled; instead, as Dmitry has commented above, one must instead use js-mode with js2-minor-mode enabled.

Feature Wishlist

In order for complete support for JSX to be available in js2-mode (not just js2-minor-mode), the following things should be done:

  • The js2-mode parser should parse and add to its AST JSX nodes (not just E4X nodes; perhaps JSX support should even replace E4X support)
  • The js2-mode parser should add text properties that the js-mode JSX indentation support (introduced in Emacs 27) can use to indent code properly
  • The js2-mode parser should add font-lock text properties for highlighting JSX code properly
  • js2-mode could auto-detect JSX like js-mode does

As has been pointed out by Stefan in our email thread—and I’m in agreement with him on this—long term, it may be best to set text properties related to indentation using syntax-propertize in js2-mode as well, rather than relying on the AST. This would be good for performance because it would reduce the time spent after a keystroke that must elapse in order to parse the buffer sufficiently to determine how to indent nearby code (since syntax-propertize is chunk-based, unlike AST parsing).

Therefore, another long-term change we may wish to work towards would be:

  • js2-mode should use js-mode’s syntax-propertize-function and not set indentation-related text properties during parsing under any conditions

However, before we make that leap, we’d like to ensure that this change will be compatible with derivative modes (like @felipeochoa’s rjsx-mode) which set syntax-table properties using the AST. Of particular interest is this line from rjsx-mode.

The Future of rjsx-mode

I recognized that users were finding it challenging to work with JSX code in Emacs and also with configuring Emacs to do that. Therefore, I resolved it would be best to include support for JSX in Emacs’ core, and that work has already been compeleted. What does this mean for rjsx-mode?

  1. Indentation and highlighting of JSX are both available to js-mode users (using Emacs 27) without needing to install rjsx-mode
  2. People who use js2-mode (rather than js2-minor-mode) still need to install rjsx-mode to get indentation/highlighting
  3. rjsx-mode still extends the js2-mode parser to parse JSX properly (enabling js2-mode linting of JSX code, et al)
  4. rjsx-mode still provides electricity while typing tags

Given these points, I can envision the following futures for this package:

  1. An effort is made to merge rjsx’s JSX parsing into js2-mode, thus point 3 (above) is obsolete; also, this creates the conditions for point 2 (above) to be resolved
  2. No effort is ever made to add JSX parsing to js2-mode, and…
    1. Users gradually migrate away from JS2+RJSX and just use js-mode, or
    2. Users continue to install JS2+RJSX and we keep the status quo (which in my opinion is overly complex)

Assuming future 1 is pursued, rjsx-mode’s purpose could transition to be to provide interesting React/JSX features like electricity. Or, electricity could also be migrated to JS2 or even to js-mode, and rjsx-mode could be retired.

But those are just my ideas, I would like to hear from @felipeochoa what he thinks of them, and how he thinks the future should unfold for his package.

@UwUnyaa
Copy link

UwUnyaa commented May 28, 2019

Relying on imports sounds like an awful idea to me, there are many libraries which rely on JSX, and looking only React (down to import names) is just a bad idea, especially because it doesn't take the possibility of a different JSX pragma (and another one for fragments) into account. These problems might crop up even in React projects that use something like @emotion/core, which provides its own JSX pragma.

In fact, making JS2 pragma-aware would allow to catch errors where the user forgets to import the pragma (which for React projects would be React.createElement).

@jacksonrayhamilton
Copy link
Contributor Author

@DoMiNeLa10 Since React is the bearer of JSX and a React variable usually needs to be in scope in order for React.createElement calls to work, I don’t think it’s a terrible starting point.

Also, Emacs 27 makes available a customizable variable called js-jsx-regexps. Its default value is ("\\_<\\(?:var\\|let\\|const\\|import\\)\\_>.*?React"). Based on my reading of emotion’s homepage, I think you could add "@jsx jsx" or "@emotion/" to that list to improve JSX detection for your project.

@jacksonrayhamilton
Copy link
Contributor Author

Since the spirit of the changes provided by this PR won’t be merged, I’ll close it.

There are still a few changes in relation to the addition of JSX that I think should be addressed:

  • Documentation within the package (rather than just the README)
  • js2-jsx-mode doesn’t even indent properly anymore in Emacs 27 (nor would I expect it to, but we should figure out what to do about that)

Also, several threads could be spun from my big post above.

I’ll create separate issues / PRs for all of these things.

@steinarb
Copy link

@dgutov As an emacs 26 user, It would be nice if all of the JSX PRs could be merged into js2-mode and a new release of js2-mode made to melpa. If so, I could throw out rjsx-mode and use js2-mode instead.

@dgutov
Copy link
Collaborator

dgutov commented Oct 20, 2019

There's no open PR that will make it all magically work in Emacs 26.

@jacksonrayhamilton
Copy link
Contributor Author

Rather, there are a number of open issues offering specifications for merging rjsx-mode and js2-mode: #527, #528, #529, #530

The rjsx-mode author is okay with his project being merged into this one. The community simply needs a volunteer to step up and do the work.

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.

JSX quotation bug Highlighting & indenting embedded XML (E4X)?
4 participants