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

chore: Toolkit migration to v4.x.x #529

Merged
merged 1 commit into from
Jan 24, 2025
Merged

chore: Toolkit migration to v4.x.x #529

merged 1 commit into from
Jan 24, 2025

Conversation

Nazehs
Copy link
Contributor

@Nazehs Nazehs commented Jan 15, 2025 •
edited
Loading

Description

This PR is migrating the package to dotcom toolkit v4.x.x

Ticket

Jira Ticket

What is the new version number in package.json?

Link to the next-syndication-dl PR which bumps the next-syndication-api version

@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-kmjtdp January 15, 2025 13:31 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 15, 2025 14:37 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 15, 2025 14:38 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 15, 2025 16:35 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 15, 2025 16:36 Inactive
@Nazehs Nazehs force-pushed the toolkit-migration branch from 6e537d3 to 4be191d Compare January 15, 2025 16:39
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 15, 2025 16:40 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 16, 2025 10:27 Inactive
@Nazehs Nazehs force-pushed the toolkit-migration branch from b673651 to 8814962 Compare January 16, 2025 10:41
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 16, 2025 10:41 Inactive
@Nazehs Nazehs force-pushed the toolkit-migration branch from 8814962 to 08fe0ff Compare January 16, 2025 10:51
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 16, 2025 10:51 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 16, 2025 10:53 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 20, 2025 13:40 Inactive
@Nazehs Nazehs force-pushed the toolkit-migration branch from 41fbcf7 to ca36604 Compare January 20, 2025 13:54
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 20, 2025 13:55 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 20, 2025 14:10 Inactive
@Nazehs Nazehs force-pushed the toolkit-migration branch from 6641446 to 64cecb5 Compare January 20, 2025 14:12
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 20, 2025 14:13 Inactive
@Nazehs Nazehs marked this pull request as ready for review January 20, 2025 14:22
asugar13
asugar13 reviewed Jan 20, 2025
View reviewed changes
package.json Outdated
"@financial-times/n-es-client": "4.1.0",
"@dotcom-reliability-kit/crash-handler": "^4.1.12",
"@dotcom-reliability-kit/middleware-log-errors": "^4.2.6",
"@financial-times/n-es-client": "6.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: There are many dependencies being bumped and enough changes due to the tool-kit migration in this PR. Do we need to bump up n-es-client or eslint-config-next or n-test for the tool-kit migration to be done properly?

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 is me upgrading internal dependencies on the app. it has nothing to do with toolkit working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: If the upgrade on dependencies is not related to the toolkit upgrade I think it would be better to leave it in another separate PR (dependency upgrade for node 22 migration would be a good candidate since it needs to upgrade those dependencies as well)

asugar13
asugar13 reviewed Jan 20, 2025
View reviewed changes
package.json Outdated
@@ -56,7 +57,7 @@
"node-pandoc": "^0.3.0",
"nodemon": "^3.1.9",
"nyc": "^11.0.2",
"pm2": "^2.6.1",
"pm2": "^5.4.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Same suggestion as before. This is bumping 3 major versions in the same PR that does the tool-kit migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bump up the pm 2 version because is the existing version was not working after the changes on node version and toolkit.

asugar13
asugar13 reviewed Jan 20, 2025
View reviewed changes
package.json Outdated
@@ -104,7 +105,7 @@
]
},
"engines": {
"node": "18.x"
"node": "22.x"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Wasn't this on node 22 already? I'd suggest waiting until the node22 migration is done before merging the toolkit migration.

Copy link
Contributor Author

@Nazehs Nazehs Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The initial changes for node migration were reverted by @ytcleon with the assumption that it was the cause of the recent syndication incident so i sneak this back since the only place that will be left after toolkit migration will the package.json

Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is the node 22 migration necessary for the tool kit migration?

asugar13
asugar13 reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@asugar13 asugar13 left a comment

Choose a reason for hiding this comment

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

This is looking good but I left some suggestions/considerations.

@Nazehs Nazehs force-pushed the toolkit-migration branch from 64cecb5 to d1730c8 Compare January 21, 2025 16:35
@next-team next-team had a problem deploying to ft-next-synd-toolkit-mi-jvlwng January 21, 2025 16:36 Failure
@Nazehs Nazehs force-pushed the toolkit-migration branch from d1730c8 to 340e550 Compare January 21, 2025 16:44
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-jvlwng January 21, 2025 16:44 Inactive
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-gcvccy January 21, 2025 16:59 Inactive
@Nazehs Nazehs force-pushed the toolkit-migration branch from 340e550 to 1853939 Compare January 22, 2025 10:38
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-gcvccy January 22, 2025 10:39 Inactive
@Nazehs Nazehs force-pushed the toolkit-migration branch from 1853939 to 55aea6b Compare January 22, 2025 12:34
@next-team next-team temporarily deployed to ft-next-synd-toolkit-mi-gcvccy January 22, 2025 12:34 Inactive
@Nazehs Nazehs merged commit cee8d58 into main Jan 24, 2025
9 checks passed
@Nazehs Nazehs deleted the toolkit-migration branch January 24, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asugar13 asugar13 asugar13 left review comments

@ytcleon ytcleon ytcleon approved these changes

@ft-dependency-auto-merger ft-dependency-auto-merger Awaiting requested review from ft-dependency-auto-merger ft-dependency-auto-merger is a code owner

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

Successfully merging this pull request may close these issues.
4 participants
@Nazehs @asugar13 @ytcleon @next-team