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

[ACC-1242] Update to npm@7 #100

Merged
merged 1 commit into from
Jan 11, 2022
Merged

[ACC-1242] Update to npm@7 #100

merged 1 commit into from
Jan 11, 2022

Conversation

aasanchez
Copy link
Contributor

Scope:
Upgrade to npm7

Changes:

  • Need to remote 2 jobs in circleci, related to Old OpenSSH version on CircleCI
  • Install python in the node container, this is a solution to this fail in the build
  • Refactor in the README.md

@aasanchez aasanchez requested review from a team and binaryberry December 21, 2021 14:15
@aasanchez
Copy link
Contributor Author

I need information about the release process for this package, I can see to be able to deploy this, is need to set a new version.

My question is, maybe this will bring breaking changes to other projects, we should jump to version 6.x.x or keep in 5.x.x

binaryberry
binaryberry reviewed Dec 21, 2021
View reviewed changes
Copy link

@binaryberry binaryberry left a comment

Choose a reason for hiding this comment

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

Looks good to me as far as I can tell! And congrats on the 100th n-syndication PR. It will require a release of n-syndication - please get someone to show you how we release if you haven't done it at the FT so far.

@aasanchez aasanchez changed the title chore: Update to npm 7 [ACC-1242] chore: Update to npm 7 Dec 21, 2021
@aasanchez aasanchez changed the title [ACC-1242] chore: Update to npm 7 [ACC-1242] chore: Update to npm@7 Dec 21, 2021
@aasanchez aasanchez changed the title [ACC-1242] chore: Update to npm@7 [ACC-1242] Update to npm@7 Dec 21, 2021
@aasanchez aasanchez force-pushed the ACC-1242-upgrade-to-npm7 branch 5 times, most recently from fd69e81 to 8e60349 Compare December 22, 2021 11:22
polkwc
polkwc reviewed Dec 22, 2021
View reviewed changes
package.json
@@ -36,21 +37,23 @@
"n-ui-foundations": "github:financial-times/n-ui-foundations#nobower",
"next-session-client": "^3.0.1",
"o-buttons": "npm:@financial-times/o-buttons@^6.0.2",
"o-icons": "npm:@financial-times/o-icons@^6.3.0",
Copy link

Choose a reason for hiding this comment

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

Question; Is this correct? To move from 7 to 6.3 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an automatic change, I didn't touch this... must be something related to dependencies

package.json
@@ -20,6 +20,7 @@
"babel-preset-react": "^6.11.1",
"bower": "^1.8.8",
"bower-resolve-webpack-plugin": "^1.0.3",
"check-engine": "^1.10.1",
Copy link

Choose a reason for hiding this comment

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

Question; don't you have to add the package.lock.json too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Becuase is ignored in .gitignore

polkwc
polkwc reviewed Dec 22, 2021
View reviewed changes
.circleci/config.yml
- run:
name: shared-helper / npm-install-peer-deps
command: .circleci/shared-helpers/helper-npm-install-peer-deps
- run:
Copy link

Choose a reason for hiding this comment

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

Question; it is correct to remove this job? (https://github.com/Financial-Times/session-decoder-js/blob/49bd53b8896b7cc5efd9c65a2ca651f348b25505/.circleci/config.yml#L78)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is a know problem in the npm migration, is documented here https://github.com/financial-times/next/wiki/npm-7-Migration-Guide#old-openssh-version-on-circleci and was apply like here https://github.com/Financial-Times/next-ammit-api/pull/796/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47L74

@aasanchez aasanchez force-pushed the ACC-1242-upgrade-to-npm7 branch 5 times, most recently from e0d7d38 to 979665b Compare January 4, 2022 00:21
@aasanchez aasanchez closed this Jan 4, 2022
@aasanchez aasanchez force-pushed the ACC-1242-upgrade-to-npm7 branch from 979665b to 9604189 Compare January 4, 2022 00:25
@aasanchez aasanchez reopened this Jan 4, 2022
@aasanchez aasanchez force-pushed the ACC-1242-upgrade-to-npm7 branch 3 times, most recently from 495d11b to a7a307f Compare January 4, 2022 00:46
@aasanchez aasanchez force-pushed the ACC-1242-upgrade-to-npm7 branch from a7a307f to 8e89e2b Compare January 4, 2022 00:46
@aasanchez aasanchez merged commit ed55ff1 into main Jan 11, 2022
@aasanchez aasanchez deleted the ACC-1242-upgrade-to-npm7 branch January 11, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@polkwc polkwc polkwc left review comments

@binaryberry binaryberry binaryberry left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@aasanchez @binaryberry @polkwc