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

Feat/lexical binding #37

Merged
merged 20 commits into from
Oct 19, 2024
Merged

Conversation

joshbax189
Copy link
Contributor

@joshbax189 joshbax189 commented Oct 18, 2024

Summary

Fixes #36
Enables lexical binding, removes uses of dynamic binding and fixes some issues arising from those changes.

Major changes

  • js-comint-repl (cmd) signature change to js-comint-repl (&optional cmd),
    This better reflects intended usage
  • js-comint-setup-module-paths removed, functionality merged into js-comint-start-or-switch-to-repl
  • js-comint-start-or-switch-to-repl now creates a temporary environment rather than doing setenv every run
  • Change package-requires to emacs 28.1 to use with-environment-variables macro
  • js-comint-repl now sets the command and uses nvm without using a prefix arg
    Previously these were included in the when clause within the interactive call, meaning that they were almost never used.
    This probably was an indentation/nesting error. The effect is that when cmd is given, the global js-comint-program-command will be updated.

Questions

  • Is the new behaviour of js-comint-repl correct/sensible?
    Check use with and without nvm
  • Is the emacs 28.1 constraint acceptable?
    (IMO emacs LTS version is 29, so it's ok)

Specifics

After enabling lexical binding in the main file these functions had "unused lexical variable" warnings

  • js-comint-filter-output (string)
    Fixed by marking the arg as ignored.
  • js-comint-repl (cmd)
    Fixed by moving the "set command" logic out of the interactive call.

As noted above, the behaviour of js-comint-repl (cmd) is now sticky:

(setq js-comint-program-command "node")
(js-repl "custom-node") ;; uses custom-node
js-comint-program-command ;; custom-node
(js-repl) ;; uses custom-node

If nvm is used:

(js-comint-select-node-version) ;; manually select node-123
(js-repl "custom-node") ;; uses custom-node, prints message warning nvm is disabled
js-comint-program-command ;; custom-node
js-use-nvm ;; nil
(js-repl)  ;; uses custom-node

Previously, these effects only happened when js-comint-repl was called interactively with a prefix argument.

Also, within js-comint-repl the variable js-comint-module-paths is dynamically let-bound to temporarily change its value (used in js-comint-setup-module-paths). This required more extensive changes to fix.

Since js-comint-setup-module-paths sets the environment permanently using setenv, the dynamic binding creates a bug where every new visited node_modules is added to the NODE_PATH for each successive call to js-comint-repl. Instead I changed to using a local environment for each call to js-comint-start-or-switch-to-repl.

Using a local environment uncovers a different problem:

(js-comint-repl) ;; adds /local/node_modules to NODE_PATH
;; calls js-comint-start-or-switch-to-repl
(js-comint-reset-repl) ;; never calls js-comint-suggest-path
;; calls js-comint-start-or-switch-to-repl

When NODE_PATH was set globally, this was fine as the old path was still present. But now the environment is reset for each call to js-comint-start-or-switch-to-repl. Better for the dynamic setting behaviour to move from js-comint-repl to js-comint-start-or-switch-to-repl so that successive calls still use the expected node_modules.

It just changes the output in the comint buffer.
require does not load if already present.
Convert cond to equivalent if
This means that default-directory is always added to NODE_PATH
when calling js-comint-repl
This avoids modifying NODE_PATH for later calls
Made cmd optional and documented behaviour.
read-directory-name defaults to default-directory if nil
and expand-file-name produces output equivalent to file-truename.
This way js-rest-repl does not "forget" the local node_modules.
This fixes byte-compiler warnings, but keeps nvm optional
js-comint-select-node-version is the common entry point when using
nvm functions.
@redguardtoo redguardtoo merged commit 103e574 into redguardtoo:master Oct 19, 2024
2 checks passed
@redguardtoo
Copy link
Owner

Thanks

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.

Enable lexical binding
2 participants