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.

Sign up for GitHub

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

Jump to bottom

BREAKING CHANGE: dropping support for eof node versions 12, 14 and 16; upgrade dependencies #605

Merged
merged 1 commit into from
Sep 14, 2023
Merged

BREAKING CHANGE: dropping support for eof node versions 12, 14 and 16; upgrade dependencies #605

merged 1 commit into from
Sep 14, 2023

Conversation

aorinevo
Copy link
Collaborator

@aorinevo aorinevo commented Sep 11, 2023 •
edited
Loading

Changes

  • Update CI dropping support for EOL Node versions 12, 14, and 16.
  • Bump Release Node version to 18, latest supported Node version.

@aorinevo aorinevo mentioned this pull request Sep 11, 2023
Closed
@aorinevo aorinevo force-pushed the master branch 2 times, most recently from 53fc79f to d214686 Compare September 11, 2023 18:23
aorinevo
aorinevo commented Sep 11, 2023
View reviewed changes
.eslintrc.js Outdated
@@ -17,6 +17,7 @@ module.exports = {
},
rules: {
'@typescript-eslint/no-unsafe-assignment': 'off',
'@typescript-eslint/no-unsafe-argument': 'off',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To handle the pervasive use of implicit any everywhere. We should follow-up with proper typing but out-of-scope of this PR as it is an existing issue and scope is narrowed down to dependency management at this time.

aorinevo
aorinevo commented Sep 11, 2023
View reviewed changes
.github/workflows/release.yml
@@ -19,7 +19,7 @@ jobs:
- name: Setup Node.js
uses: actions/setup-node@v2
with:
node-version: 12
node-version: 18
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bumping to minimum supported version.

aorinevo
aorinevo commented Sep 11, 2023
View reviewed changes
.github/workflows/ci.yml
@@ -5,7 +5,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
node: ['12', '14', '16']
node: ['18', '20']
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As mentioned in the description, versions < 18 are EOL as of 9/11/2023.

aorinevo
aorinevo commented Sep 11, 2023
View reviewed changes
src/util/migration-spec.ts
@@ -26,7 +26,7 @@ export interface IMigrationSpec {

export function loadSpec(directory: string): IMigrationSpec {
const docPath = path.join(directory, 'shepherd.yml');
const spec = yaml.safeLoad(fs.readFileSync(docPath, 'utf8'));
const spec = yaml.load(fs.readFileSync(docPath, 'utf8'));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yaml package renamed safeLoad to load. In this PR, we update to the new name.

aorinevo
aorinevo commented Sep 11, 2023
View reviewed changes
src/util/persisted-data.ts
@@ -16,7 +16,7 @@ const jsonStringify = (data: any) => JSON.stringify(data, undefined, 2);
const migrateToJsonIfNeeded = async (migrationContext: IMigrationContext) => {
const legacyFile = getLegacyRepoListFile(migrationContext);
if (await fs.pathExists(legacyFile)) {
const data = yaml.safeLoad(await fs.readFile(legacyFile, 'utf8'));
const data = yaml.load(await fs.readFile(legacyFile, 'utf8'));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as previous comment.

@aorinevo
Copy link
Collaborator Author

aorinevo commented Sep 11, 2023 •
edited
Loading

Need to ensure the major version gets bumped as dropping support for Node versions is a breaking change.

@aorinevo aorinevo force-pushed the master branch 2 times, most recently from de207d0 to 38355d3 Compare September 11, 2023 21:39
@aorinevo aorinevo changed the title chore(node): dropping support for eof node versions 12 and 14 chore(node): dropping support for eof node versions 12, 14 and 16 Sep 11, 2023
aorinevo
aorinevo commented Sep 13, 2023
View reviewed changes
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@nerdwallet/shepherd",
"version": "1.16.0",
"version": "2.0.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This manual bump is unnecessary as semantic-release will bump the major version.

@aorinevo aorinevo force-pushed the master branch 2 times, most recently from 1757a44 to 04be4ca Compare September 13, 2023 01:46
aorinevo
aorinevo commented Sep 13, 2023
View reviewed changes
src/logger/index.ts Outdated
this.oraInstance = ora(message).start();
this.spinnerActive = true;
this.oraInstance.start();
import('ora')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This dynamic import is a workaround and not a good one. It causes race conditions that are more challenging to solve than to convert to using a different spinner. Need to evaluate alternatives.

aorinevo
aorinevo commented Sep 13, 2023
View reviewed changes
src/logger/index.ts Outdated
@@ -58,28 +56,34 @@ export default class ConsoleLogger implements ILogger {
};

public succeedIcon = (message: string): void => {
this.info(`${logSymbols.success} ${message}`);
this.info(`✅ ${message}`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One option to removing dependency on log-symbols which, as in the case of ora, requires dynamic imports. I'm fine with using emoji, unless there's a preference or reason to do otherwise.

@aorinevo
Copy link
Collaborator Author

Almost there. Local testing looks good but there are a few issues left outstanding that need to be resolved:

  1. Deprecate use of log-symbols in favor of similar TS module or just use emoji
  2. Deprecate ora spinner in favor of similar TS module

I'm leaning towards deprecation of both log-symbols and ora. Open to feedback :)

@aorinevo aorinevo force-pushed the master branch 3 times, most recently from 45460bb to 16a094e Compare September 14, 2023 11:28
@aorinevo
Copy link
Collaborator Author

This PR is ready to ship. Unit tests pass as before and tested local as well.

aorinevo
aorinevo commented Sep 14, 2023
View reviewed changes
.eslintrc.js
@@ -16,17 +16,19 @@ module.exports = {
sourceType: 'module',
},
rules: {
'@typescript-eslint/no-unsafe-assignment': 'off',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Prefer integer value. 0 = off, 1 = warn, and 2 = error.

@aorinevo aorinevo changed the title chore(node): dropping support for eof node versions 12, 14 and 16 BREAKING CHANGE: dropping support for eof node versions 12, 14 and 16; upgrade dependencies Sep 14, 2023
@aorinevo aorinevo merged commit 26abb83 into NerdWalletOSS:master Sep 14, 2023
3 checks passed
@aorinevo aorinevo requested a review from luislew September 18, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nwalters512 nwalters512 Awaiting requested review from nwalters512

@luislew luislew Awaiting requested review from luislew

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@aorinevo