-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[WIP] chore(deps): switch to yargs-parser lib #3377
Conversation
7c2842b
to
789abbe
Compare
It seems Rollup CLI is very much broken with this update, see the test results. Can you try to fix this? |
[!] ReferenceError: path is not defined
ReferenceError: path is not defined
at external (/home/circleci/project/cli/run/loadConfigFile.ts:24:24)
at configExternal.function.rest (/home/circleci/project/src/utils/mergeOptions.ts:81:5)
at ModuleLoader.args [as isExternal] (/home/circleci/project/src/ModuleLoader.ts:52:52)
at ModuleLoader.<anonymous> (/home/circleci/project/src/ModuleLoader.ts:202:9)
at Generator.next (<anonymous>)
at /home/circleci/project/node_modules/tslib/tslib.es6.js:73:71
at new Promise (<anonymous>)
at Object.__awaiter (/home/circleci/project/node_modules/tslib/tslib.es6.js:69:12)
at ModuleLoader.resolveId (/home/circleci/project/dist/rollup.js:12187:22)
at ModuleLoader.<anonymous> (/home/circleci/project/src/ModuleLoader.ts:247:39) This is what the CI complains about. |
Looks non-trivial. It seems like the presence of yargs-parser is messing up the path import. You can see this when you inspect dist/bin/rollup. Not sure how this is even possible, might be something messed up by the CommonJS plugin. This will require some digging. |
Seems like using named imports for 'path' fixes it. Now there is only one broken test that needs further inspection. Basically it seems that config files can no longer edit the command arguments. |
# Conflicts: # LICENSE.md # cli/run/loadConfigFile.ts # package-lock.json # package.json # src/watch/watch.ts
…gs-parser # Conflicts: # cli/run/loadConfigFile.ts
deb8cff
to
2f336f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that the remaining issue was due to the automatic camel-case-expansion which I now disabled. So from my side this is ready to be merged now.
Codecov Report
@@ Coverage Diff @@
## master #3377 +/- ##
==========================================
+ Coverage 93.29% 95.01% +1.71%
==========================================
Files 172 171 -1
Lines 6114 5834 -280
Branches 1823 1722 -101
==========================================
- Hits 5704 5543 -161
+ Misses 218 157 -61
+ Partials 192 134 -58
Continue to review full report at Codecov.
|
@lukastaegert thanks for the help 👏 |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Switch over to
yargs-parser
for the purpose of argument parsing. Closes #3376