Refactor codebase for better scalability #161
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
For both modules, define a `Map` that maps the names of shell binaries to a respective function that escapes arguments for that shell. For win.js this amounts to replacing the if-else-statement in `escapeShellArg` by logic that obtains the escape function from the map. For unix.js more changes were required. In contrast to win.js the unix module wasn't yet set up to separate argument escaping by shell. Hence, first the individual escape functions are rewritten such that there is one per (group of) shell(s). Second, the same map-related logic from win.js is added to the `escapeShellArg` function of the unix module. Both of these implementations use the standard NodeJS `path` package to reliably obtain the name of the shell binary from the provided shell, which may be a full path. As this is a new import, the Rollup config is updated to mark it as an external dependency.
- For unix shells, replace "/bin/sh" by "/bin/dash" to align with the officially supported shells (see README). The `unixShells` array is updated accordingly. - For windows shells, use full paths to more accurately represent what these values would be in a real scenario.
Codecov Report
@@ Coverage Diff @@
## main #161 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 6 6
Lines 499 701 +202
==========================================
+ Hits 499 701 +202
|
Remove the near-identical `escapeShellArg` functions from unix.js and win.js and incorperate it in main. This simplifies both the unix.js and win.js modules and their respective test suites. the main.js module had to undergo some changes to accomodate a generic `escapeShellArg` function and the implementation here only aims to be the cleanest translation from its previous iteration.
Reduce the cognitize complexity and number of functions in main.js following the design of the `resolveExecutable` function. A new function is added, `escapeShellArg` that is explicitly provided some functions that can come from eiterh the unix.js or win.js module. By doing this, most of the branches related to the platform are now gone (one is kept because it's for a feature that is marked for removal in the next major version). To accomodate this, the unix.js and win.js now expose a `getBasename` function. Additionally, unix.js and win.js were refactored to explose a function called `getEscapeFunction` instead of the `escapeFunctionsByShell` map directly, primarly to avoid accidentally changing the map outside of the respective modules. All existing tests were changed to accomodate these changes. The `getBasename` functions are independently tested as well.
This is a temporary solution that should avoid mismatches between the values in main.js and the values in win.js and unix.js.
...for unsupported shells specifically.
Refactor main.js, and unix.js and win.js accordingly, to be able to use a different function for quoting an argument based on the selected shell (rather than just the platform). All changes are fully reflected in the test suites and all changes to the behaviour of functions are tested completely.
- Remove the "null character" tests as there's no guarentee this should hold for all shells. - Test that `null` is returned for all unsupported shell to avoid any shell(Name) from accidentally being supported.
Instead of writing `Object.assign` in-line, create a helper functions with a clear name of what is happening to improve readability. The implementation also prevents any possible prototype polution [1] by assigning the merge result to a object created by `Object.create(null)`. -- 1. https://learn.snyk.io/lessons/prototype-pollution/javascript/
- Remove property test from unix.prop.js that has just three possible values. This is tested exhaustively in the unit tests. - Update the property tests for the shell fallback of both Unix and Windows to take into account that the generated value could be a (sub)path.
ericcornelissen
added
the
refactor
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.
No description provided.