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

Fix for double characters due to inquirer package: #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CodexHere
Copy link

  • Updated minimal dependencies to get operational:
    • nats
    • inquirer - the main culprit of double characters
    • eslint and associative plugins
    • jest for experimental VM Module import support
    • npm-check - was missing but used in npm scripts
  • Updated eslintrc to refer to ECMA 2022 for dynamic import features used within
  • Updated importing inquirer using dynamic import
    • Required a few changes to keep order of operations in check, mainly awaiting import adhoc.
  • With changes to using a dynamic import, JEST required node to be executed with NODE_OPTIONS=--experimental-vm-modules
  • Removed a few unused references in files touched
  • Updated create.spec to mock with changes due to dynamic import, and doing so in a beforeEach functional block to remove redundancy

* Updated minimal dependencies to get operational:
	* nats
	* inquirer - the main culprit of double characters
	* eslint and associative plugins
	* jest for experimental VM Module import support
	* npm-check - was missing but used in npm scripts
* Updated eslintrc to refer to ECMA 2022 for dynamic import features
  used within
* Updated importing `inquirer` using dynamic import
	* Required a few changes to keep order of operations in check,
	  mainly `await`ing import adhoc.
* With changes to using a dynamic import, JEST required node to be
  executed with `NODE_OPTIONS=--experimental-vm-modules`
	* See: https://jestjs.io/docs/ecmascript-modules
* Removed a few unused references in files touched
* Updated create.spec to mock with changes due to dynamic import, and
  doing so in a `beforeEach` functional block to remove redundancy
@icebob
Copy link
Member

icebob commented Jul 12, 2023

This PR contains a lot of breaking changes. Can we upgrade only the non-ESM inquirer 8.x.x? Is it solved the original problem?

@AndreMaz
Copy link
Member

I don't think so. This seems to be the source of the problem: SBoudrias/Inquirer.js#109 and I think that this was only solved in v9 of inquirer

@CodexHere
Copy link
Author

Ah a few of us had kinda talked about this in the Discord...

As @AndreMaz mentions, this was only fixed after it moved to ESM.

I wasn't sure how the legacy node versions were handled, and I tested down to v16 due to it being the lowest version still not EOL'd (although in 2 months it will be, and it's the lowest version dockerhub has in their registry).

From the minimal error information, it seems like some dependency is failing it's postinstall script while accessing an arbitrary object. async/await was added in node 7.6, so not sure which lib would be dealing with this.

I don't currently have the capacity to build Dockerfiles for v10-14 for node, but might in the near™ future so that I can dive deeper into which dependency is failing

Otherwise, not sure what the course of action is here.

@icebob
Copy link
Member

icebob commented Jul 14, 2023

I don't think so. This seems to be the source of the problem: SBoudrias/Inquirer.js#109 and I think that this was only solved in v9 of inquirer

I've checked the issue, but I don't see it's been solved, no any relevant commit linked to the issue, just workarounds in the comments.

@AndreMaz
Copy link
Member

You are right, the issue doesn't contain any fix for the issue. However, I've tested locally by bumping inquirer to latest v8.x version and the issue was still present. In this PR the issue is gone

You can reproduce the issue by running npm run dev

@CodexHere
Copy link
Author

On that issue the dev says he fixed a bunch of event listeners and it's working. Unfortunately there's no explicit commit to knowingly point to: SBoudrias/Inquirer.js#109 (comment)

@ImTheDeveloper
Copy link

Came here as I have the double char issue also. Did anything get merged in or a work around? I can see this one is failing some tests now.

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.

4 participants