Use a Promise wrapper for fs actions and minimist for argv parsing #25
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
Errors while reading and writing the namespace were not passed on to the Promise chain, so the script would report that it was complete prior to actually finishing file system interactions (where a further error may then occur after we gave the all-clear). This commit wraps the callback- based filesystem interaction in a Promise and passes errors to reject(). Chalk is also added to the project dependencies so that we don't rely on the react-scripts dependency list.
While defaulting to ReactWPScripts does run the risk of namespace collisions if multiple themes or plugins use the default value, deriving the name of the script-loader namespace from the project's directory name feels overly-general.
Merged
tfrommen
reviewed
Mar 8, 2018
scripts/init.js
Outdated
| namespace = upperCamelCase( projectPackageJSON.name ); | ||
| } | ||
| catch ( err ) {} | ||
| let namespace = argv['php-namespace'] || 'ReactWPScripts'; |
|
@kadamwhite Can you split your php-namespace change out? The rest seems uncontroversial, so we can merge that straight away :) |
rmccue
approved these changes
Mar 9, 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.
This PR (which is PR'd against #15, not against master) proposes some final changes to #15 based on my review comments there.
fsactions in a ¶romise to ensure that action completes before we report successchalkto project dependencies--php-namespacecommand-line argumentTo test, run