Skip to content

refactor: migrate spinner to use import/export - #2366

Merged
1 commit merged into
masterfrom
refactor/spinner-esm
Nov 18, 2021
Merged

refactor: migrate spinner to use import/export#2366
1 commit merged into
masterfrom
refactor/spinner-esm

Conversation

@ghost

@ghost ghost commented Nov 16, 2021

Copy link
Copy Markdown

Considering NodeJS more or less supports ES modules now natively, I was checking to see how close we are to building CLI using ES modules. The only thing stopping us TypeScript-wise seems to be the spinner module. So I've migrated that. The only thing left after this is the various JS modules.

Notes

There's some odd logic around isRequired. I think this was for tests and import flows which we no longer provide. So it's no longer needed. See: #2371

The isCI check is also kind of weird. It looks like we can check the environment if it's TTY instead to decide whether or not to render the spinner. Kind of like how we render colours.

I don't want to get too deep into improving spinner's code/types as we may want to replace it with a library. Especially if we want to show better progress updates (e.g. percentages, fractions, etc.).

@github-actions

github-actions Bot commented Nov 16, 2021

Copy link
Copy Markdown
Contributor
Warnings
⚠️

Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones.
A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax.
Files found:

  • src/cli/commands/monitor/index.ts
  • src/cli/commands/protect/wizard.ts
  • src/lib/plugins/nodejs-plugin/npm-modules-parser.ts
  • src/lib/snyk-test/run-test.ts
  • src/lib/spinner.ts
⚠️ You've modified files in src/ directory, but haven't updated anything in test folder. Is there something that could be tested?
Messages
📖

This PR will not trigger a new version. It doesn't include any commit message with feat or fix.

Generated by 🚫 dangerJS against e575119

@ghost
ghost marked this pull request as ready for review November 16, 2021 13:49
@ghost
ghost self-requested a review as a code owner November 16, 2021 13:49
@ghost
ghost self-requested a review November 16, 2021 13:49
@ghost
ghost self-requested a review as a code owner November 16, 2021 13:49
Comment thread src/lib/spinner.ts Outdated
Comment thread src/lib/spinner.ts Outdated

@maxjeffos maxjeffos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Made a couple suggestions but approved it.

@ghost
ghost force-pushed the refactor/spinner-esm branch from 6d3c21f to 9f6d514 Compare November 18, 2021 17:23
@ghost
ghost force-pushed the refactor/spinner-esm branch from 9f6d514 to e575119 Compare November 18, 2021 17:28
@ghost
ghost merged commit 657e53b into master Nov 18, 2021
@ghost
ghost deleted the refactor/spinner-esm branch November 18, 2021 18:11
This pull request was closed.
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.

2 participants