Closed jacksonrayhamilton closed 5 years ago
Since I first opened the PR last night, I got some feedback via email on the code style, and made updates accordingly (a59c7b1...63b7a72).
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.
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
?
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.
I added a section to the README which should hopefully encourage some users to download a Emacs 27 snapshot and test out my improvements.
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.
Here's hoping js2-refactor
and friends don't rely on js2-mode
being used exactly. They shouldn't anyway.
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:
import React from 'react'
or var React = require('react')
appears near the beginning of a bufferThis 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.
In order for complete support for JSX to be available in js2-mode (not just js2-minor-mode), the following things should be done:
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:
syntax-propertize-function
and not set indentation-related text properties during parsing under any conditionsHowever, 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.
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?
Given these points, I can envision the following futures for this package:
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.
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
).
@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.
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:
Also, several threads could be spun from my big post above.
I’ll create separate issues / PRs for all of these things.
@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.
There's no open PR that will make it all magically work in Emacs 26.
Rather, there are a number of open issues offering specifications for merging rjsx-mode and js2-mode: https://github.com/mooz/js2-mode/issues/527, https://github.com/mooz/js2-mode/issues/528, https://github.com/mooz/js2-mode/issues/529, https://github.com/mooz/js2-mode/issues/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.
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 enterjs-jsx-mode
). These changes allow js2-mode to tap into these improvements when using Emacs on themaster
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 thesyntax-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 thesyntax-table
property here after it is cleared elsewhere.As noted below, this now also fixes #409.