Make CLI perform static analysis of config lazily #2593
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vercel#2375 adds a wrapper CLI tool that parses the ~/.hyper.js configuration file. However, the static analysis logic can't possibly handle every possible configuration file without executing it. In my case, [my configuration file][0] defines a constant at the top, which means that the `module.exports` definition isn't the first statement. This breaks the parsing logic. However, this static analysis is only necessary if one wishes to use the CLI subcommands. They're not needed if one just wants to launch hyper. Unfortunately, the existing implementation tries to read from the AST immediately when `cli/api.js` is imported. This changes it to be lazy, and uses memoization retain the same reference, allowing the AST to still be mutated in-place. [0]: https://github.com/bgw/dotfiles/blob/665e83fd7a13be675ad0ae984478d5b0de502ee1/hyper.js#L4
|
Amazing work! Thank you so much @bgw for your help 🙏 |
Stanzilla
pushed a commit
to Stanzilla/hyper
that referenced
this pull request
Jan 17, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#2375 adds a wrapper CLI tool that parses the ~/.hyper.js configuration file. However, the static analysis logic can't possibly handle every possible configuration file without executing it.
In my case, my configuration file defines a constant at the top, which means that the
module.exportsdefinition isn't the first statement. This breaks the parsing logic.However, this static analysis is only necessary if one wishes to use the CLI subcommands. They're not needed if one just wants to launch hyper.
Unfortunately, the existing implementation tries to read from the AST immediately when
cli/api.jsis imported. This changes it to be lazy, and uses memoization retain the same reference, allowing the AST to still be mutated in-place.This PR should be ready to merge. I've tested it to the best of my ability by building a new version with
yarn run dist, modifying my configuration file, and seeing that a fewhypersubcommands seem to work.