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

Replace internal inquirer with @serverless/inquirer #7729

Merged
merged 2 commits into from May 26, 2020

Conversation

AhmedFat7y
Copy link
Contributor

Closes: #7690

@AhmedFat7y
Copy link
Contributor Author

@medikoo
Object Destructing is not supported in node v6.x
@serverless/inquirer is using a higher version of chalk ^3.0.0 which needs a higher version node engine >= 8
Other references are using ^2.4.2, ^2.4.1, ^2.0.1

/home/travis/build/serverless/serverless/node_modules/@serverless/inquirer/node_modules/chalk/source/index.js:103
	...styles,
	^^^
SyntaxError: Unexpected token ...
    at createScript (vm.js:56:10)
    at Object.runInThisContext (vm.js:97:10)
    at Module._compile (module.js:549:28)
    at Object.Module._extensions..js (module.js:586:10)
    at Module.load (module.js:494:32)
    at tryModuleLoad (module.js:453:12)
    at Function.Module._load (module.js:445:3)
    at Function.Module._load (/home/travis/build/serverless/serverless/node_modules/mock-require/index.js:30:22)
    at Module.require (module.js:504:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/travis/build/serverless/serverless/node_modules/@serverless/inquirer/index.js:8:15)
    at Module._compile (module.js:577:32)
    at Object.Module._extensions..js (module.js:586:10)
    at Module.load (module.js:494:32)

@medikoo
Copy link
Contributor

medikoo commented May 14, 2020

@AhmedFat7y indeed, thanks for finding this out. It's a bug in @serverless/inquirer which I've just fixed. Bump version to v1.1.1 and it should be good.

@AhmedFat7y
Copy link
Contributor Author

@medikoo one more thing, will this cause a problem? It seems to be path-based resolution

const promptPath = require.resolve('tabtab/lib/prompt');
const tabtabsInquirerPath = resolve(path.dirname(promptPath), 'inquirer').realPath;

@medikoo
Copy link
Contributor

medikoo commented May 15, 2020

@medikoo one more thing, will this cause a problem? It seems to be path-based resolution

This won't be affected. require.resolve resolves absolute path to inquirer as used by tabtab, and after that all is safe, as we work with correct absolute path.

@AhmedFat7y AhmedFat7y force-pushed the use-inquirer-package branch 2 times, most recently from e634cd0 to ff5b179 Compare May 15, 2020 19:18
@AhmedFat7y
Copy link
Contributor Author

@medikoo I'm not sure what's the problem here. I modified the package version to ^1.1.1 and added npm show @serverless/inquirer to travis.yml to make sure it's the same version I intended, and it's all good.

However, it still throws the same error

npm show @serverless/inquirer
@serverless/inquirer@1.1.1 | MIT | deps: 3 | versions: 3
An inquirer with enforced Serverless theme
https://github.com/serverless/inquirer#readme
keywords: inquirer, serverless
dist
.tarball: https://registry.npmjs.org/@serverless/inquirer/-/inquirer-1.1.1.tgz
.shasum: bff3d8efac69237f345c835a027b0fbd82b1d8db
.integrity: sha512-kzrFQ6/TV+z6WvMEEfOzMWEorClMrCJy4z+lcdaiEN8xe+VH+TcxT7O9tsgj4pYlHioT3raDPjTBX9jvPbzo3Q==
.unpackedSize: 16.0 kB
dependencies:
chalk: ^2.0.1    inquirer: ^7.1.0 ncjsm: ^4.0.1    
maintainers:
- serverless-main <services@serverless.com>
dist-tags:
latest: 1.1.1  
published yesterday by serverless-main <services@serverless.com>
The command "npm show @serverless/inquirer" exited with 0.
$ npm test -- -b
> serverless@1.71.1 test /home/travis/build/serverless/serverless
> mocha "!(node_modules)/**/*.test.js" "-b"
/home/travis/build/serverless/serverless/node_modules/@serverless/inquirer/node_modules/inquirer/node_modules/chalk/source/index.js:103
	...styles,
	^^^

@medikoo
Copy link
Contributor

medikoo commented May 18, 2020

@AhmedFat7y indeed, it appears there's also problem with inquirer which with patch release broken support for Node.js v6 (reported that issue: SBoudrias/Inquirer.js#920)

Anyway I've downgraded inquirer there to version which is used here, so now there should be no issue. Just change version range to ^1.1.2

@AhmedFat7y
Copy link
Contributor Author

@medikoo There's no reviewer for this PR, but it's done, what will happen now?

@codecov-commenter
Copy link

Codecov Report

Merging #7729 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7729      +/-   ##
==========================================
+ Coverage   88.11%   88.15%   +0.04%     
==========================================
  Files         246      245       -1     
  Lines        9279     9264      -15     
==========================================
- Hits         8176     8167       -9     
+ Misses       1103     1097       -6     
Impacted Files Coverage Δ
lib/plugins/interactiveCli/index.js 100.00% <100.00%> (ø)
lib/plugins/interactiveCli/initializeService.js 100.00% <100.00%> (ø)
lib/plugins/interactiveCli/setupAws.js 100.00% <100.00%> (ø)
lib/plugins/interactiveCli/tabCompletion.js 97.50% <100.00%> (ø)
lib/plugins/interactiveCli/utils.js 100.00% <100.00%> (ø)
lib/components-v2.js 80.00% <0.00%> (+40.00%) ⬆️

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 dd902e8...49a2657. Read the comment docs.

@medikoo
Copy link
Contributor

medikoo commented May 22, 2020

@medikoo There's no reviewer for this PR, but it's done, what will happen now?

Thanks for info. I'll review it next days (unfortunately in next days my priorities were shifted, so I may respond with delay now)

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @AhmedFat7y !

@medikoo medikoo merged commit 4724cb8 into serverless:master May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicated logic due to @serverless/inquirer seclusion
3 participants