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

[WIP] chore(deps): switch to yargs-parser lib #3377

Merged
merged 8 commits into from Mar 10, 2020

Conversation

jamesgeorge007
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Switch over to yargs-parser for the purpose of argument parsing. Closes #3376

@jamesgeorge007 jamesgeorge007 changed the title chore(deps): switch to yargs-parser lib [WIP] chore(deps): switch to yargs-parser lib Feb 9, 2020
@lukastaegert
Copy link
Member

It seems Rollup CLI is very much broken with this update, see the test results. Can you try to fix this?

@jamesgeorge007
Copy link
Contributor Author

[!] 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.

@lukastaegert
Copy link
Member

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.

@lukastaegert
Copy link
Member

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.

Copy link
Member

@lukastaegert lukastaegert left a 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
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #3377 into master will increase coverage by 1.71%.
The diff coverage is 98.39%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
cli/run/loadConfigFile.ts 84.00% <0.00%> (ø)
cli/run/timings.ts 0.00% <0.00%> (ø)
src/ast/nodes/ArrowFunctionExpression.ts 100.00% <ø> (ø)
src/ast/nodes/CallExpression.ts 95.41% <ø> (ø)
src/ast/nodes/Literal.ts 100.00% <ø> (ø)
src/ast/nodes/MemberExpression.ts 98.27% <ø> (ø)
src/ast/nodes/ObjectExpression.ts 91.83% <ø> (ø)
src/ast/nodes/TryStatement.ts 100.00% <ø> (ø)
src/ast/nodes/shared/FunctionNode.ts 100.00% <ø> (ø)
src/ast/nodes/shared/Node.ts 91.42% <ø> (ø)
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeda078...2f336f2. Read the comment docs.

@lukastaegert lukastaegert merged commit a468a31 into rollup:master Mar 10, 2020
@jamesgeorge007 jamesgeorge007 deleted the feat/yargs-parser branch March 12, 2020 09:40
@jamesgeorge007
Copy link
Contributor Author

@lukastaegert thanks for the help 👏

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.

[CLI]: Switch over to a less verbose argument parser library
2 participants