provide rjsx-minor-mode

[axyz]

I'm not sure if this is the cleanest way to do it or if there may be unexpected side effect.
The idea is to provide a minor-mode similar to js2-minor-mode in order to allow the usage of packages depending on the js2/rjsx AST to work in other major modes (e.g. using js2-refactor in web-mode)

I tired it locally and seems to work with js2-refactor

[felipeochoa]

Thanks for the PR!

I like the idea, but would rather leverage the existing js2-minor-mode directly. That way we're less coupled to the js2-minor-mode-enter and -exit functions and keep the coupling solely to the advising of js2-parse-xml-initializer. I think this can be achieved by using an rjsx-jsx-enabled-p variable to control rjsx-parse-xml-initializer on line 61:

(defun rjsx-parse-xml-initializer (orig-fun)
  "Dispatch the xml parser based on variable `rjsx-mode' being active or not.
This function is used to advise `js2-parse-xml-initializer' (ORIG-FUN) using
the `:around' combinator.  JS2-PARSER is the original XML parser."
  (if rjsx-jsx-enabled  ;; <------------{ CHANGE THIS }------------
      (rjsx-parse-top-xml)
    (apply orig-fun nil)))

Then simply enabling js2-minor-mode + (setq rjsx-jsx-enabled) would get you JSX parsing. rjsx-mode would have to set this variable to t on initialization, but that's not a big deal.

I'd also like to include a test for this before merging. An easy way to do this is to use js2-test-parse-string in "tests/js2-tests.el" to assert that a string is parsed and then printed correctly.

[axyz]

If I got it right, should be something like:

(defun rjsx-parse-xml-initializer (orig-fun)
  "Dispatch the xml parser based on variable `rjsx-mode' being active or not.
This function is used to advise `js2-parse-xml-initializer' (ORIG-FUN) using
the `:around' combinator.  JS2-PARSER is the original XML parser."
  (if (eq major-mode 'rjsx-mode)
      (setq rjsx-jsx-enabled-p t))
  (if rjsx-jsx-enabled-p
      (rjsx-parse-top-xml)
    (apply orig-fun nil)))

then in my mode hook I can call (js2-minor-mode) and set rjsx-jsx-enabled-p to t

Seems to work, but only if rjsx-mode was initialized before: is there a way to initialize it without something ugly like switching to rjsx-mode and then switch back to my-mode?

[felipeochoa]

If I got it right, should be something like:

I actuall meant putting the (setq rjsx-jsx-enabled-p t) directly in the (define-derived-mode ...) body. That way you won't be running this check repeatedly. Alternatively, you could change the
check to be:

(if (or (eq major-mode 'rjsx-mode) rjsx-jsx-enabled-p)
    (rjsx-parse-top-xml)
  (apply orig-fun nil))

Seems to work, but only if rjsx-mode was initialized before: is there a way to initialize it without something ugly like switching to rjsx-mode and then switch back to my-mode?

rjsx doesn't require any intialization. Calling (require 'rjsx-mode) once should be enough to
set up the advice that hooks into js2-mode's parser.

[axyz]

got it, a last minor concern is that currently the only autoload is the provided (rjsx-mode) that will actually enable the major mode.

In the js2-minor-mode case I do not want to call that function of course, but at the same time would like to autoload only the needed part for jsx parsing so that can be deferred with use-package. Do you think that it would be possible to do it? maybe wrapping the advice-add part in a function?

[felipeochoa]

but at the same time would like to autoload only the needed part for jsx parsing

Regardless of what gets an autoload cookie, the entire file is loaded when the autoload is
triggered. Conceptually, when you create a package, autoloaded definitions get translated into
something like:

(defun rjsx-mode (&rest args)
  (require 'rjsx-mode "path/to/rjsx-mode.el")
  ;; The require call overrides the defintion of `rjsx-mode'
  (apply 'rjsx-mode args))

If you call require manually, you can forget about all the autload stuff. As long as you don't
activate the major mode you won't see the keybindings activate.

(That said, since rjsx wasn't originally designed for use as a minor mode, there might be certain
side-effects during parse. These would be considered bugs and would be modified so that parsing a
file is side-effect free.)

so that can be deferred with use-package.

I'm not familiar with use-package, but if you're defining a function to enable parsing you could
do something like this:

(defun my-enable-jsx-parsing ()
  (interactive)
  (js2-minor-mode 1)
  (require 'rjsx-mode)
  (setq rjsx-jsx-enabled t))
  
(defun my-disable-jsx-parsing ()
  (interactive)
  (js2-minor-mode -1)
  (setq rjsx-jsx-enabled nil))

Or if you know you'll always want JSX parsing, even simpler:

(with-eval-after-load 'js2-mode
  (require 'rjsx)
  (setq rjsx-jsx-enabled nil))

[axyz]

basically with my latest commit I am able to load it like this:

(defun my-mode/init-rjsx-mode ()
  (use-package rjsx-mode
    :defer t
    :init
    (progn
      (add-hook 'my-mode-hook #'rjsx-mode-setup)
      )))

with

(defun rjsx-mode-setup ()
  (setq rjsx-jsx-enabled-p t)
  (rjsx-enable-parser)
  (js2-minor-mode))

this guarantees that the package gets loaded only when my-mode is activated, deferring it to the very moment (rjsx-enable-parser) is called. without the autoload it doesn't work with defer t and it will be required directly in the init. I'm not sure of the real performance benefit here, but I think the best practice would be to always allow the autoload (before was possible for rjsx-mode, but now we need a custom one only for the parsing or we are forced to require the entire thing in a blocking way).

But I'm also not that much into the topic and may be missing something.

[axyz]

in the end I think wouldn't change much as the require will happen in the hook and autoload will read the whole file anyway, I guess it is only a possible way to make it cleaner for refactors (e.g. separate one autoloaded function in a smaller file?) or to make it more explicit that you are requiring a specific function and not just blindly requiring the whole file. In this case it is pretty trivial, so if you want I can remove the autoload part and require it in the hook, or we can find a better function to export (e.g. having the autoloaded function to also set the boolean directly to have a single instruction before enabling js2-minor-mode?).

I'm not familiar with elisp testing, can I just clone the whole repo and evaluate the test file buffer?

[axyz]

@felipeochoa I'm not sure to get what do you want to test:

nothing is new on parsing side, but should be tested that using js2-minor-mode when rjsx-jsx-enabled-p is true doesn't break any test and returns the same AST.

from the test what I get is that it is just calling js2-mode and js2-reparse, while in the actual rjsx-tests.el it is doing:

(defun js2-mode--and-parse ()  ;; No point in advising, so we just overwrite this internal function
  (rjsx-mode)
  (js2-reparse))

should we add something like this?

(defun js2-minor-mode--and-parse () 
  (setq rjsx-jsx-enabled-p t)
  (js2-minor-mode) 
  (js2-reparse))

and then use it in some tests or all of them again?

[axyz]

@felipeochoa any news? I would like to use the new feature to enhance the react layer on spacemacs

[felipeochoa]

Sorry I thought I'd responded to this earlier. Just a test that does something like (with-temp-buffer (insert ...) (enable-minor-mode) (js2-parse) (should (equal something-mildly-complex (js2-print-ast)))) the test code is a bit of a mess, so no need to use the helpers that are there. Then I'll clean up everything at some point

(I'm on my phone so can't look up the exact calls, but let me know if you run into issues)

[torgeir]

Looking forward to this one! 👏

[dagadbm]

any news on this?
spacemacs react is really weak without a lot of configuration (and I am an emacs noob) compared to vscode

[axyz]

@felipeochoa I never used ert, but i tried to duplicate the js2 test helper for rjsx minor mode and seems to work, probably everything could be simplified...

[felipeochoa]

@axyz I decided to revert back to the minor mode implementation -- I think that was the right approach all along. I also cleaned up the tests a bit and combined all the commits into 8bdb234, which is in master now.

[felipeochoa]

@axyz Also, thanks for this PR! This is great functionality to add -- so sorry it took so long to merge