-
Notifications
You must be signed in to change notification settings - Fork 5.1k
feat: upgrade package manager to pnpm #15546
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
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @wackerow! thanks for the migration and lodash normalization :)
Left a comment about something related to crowdin that seems unrelated to the pnpm migration. Could you elaborate more on those changes?
@@ -62,16 +60,6 @@ export const CROWDIN_PROJECT_URL = "https://crowdin.com/project/ethereum-org" | |||
export const CROWDIN_PROJECT_ID = 363359 | |||
export const CROWDIN_API_MAX_LIMIT = 500 | |||
export const FIRST_CROWDIN_CONTRIBUTION_DATE = "2019-07-01T00:00:00+00:00" | |||
export const REGULAR_RATES: ReportsModel.RegularRate[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used? 👍🏼
@@ -25,22 +24,45 @@ export async function fetchTranslationCostsReport( | |||
|
|||
// Todo: Remove ts-ignore when this PR gets merged | |||
// https://github.com/crowdin/crowdin-api-client-js/pull/282 | |||
const schema: ReportsModel.TranslationCostSchema = { | |||
// const schema: ReportsModel.TranslationCostSchema = { | |||
const schema: ReportsModel.TranslationCostsPostEndingSchema = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't seem the PR to include this change. do we need to test something? not sure what this change is doing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some changes to the data schema required from the crowdin package. We can check with @lukassim but I'm not sure we really even use this anymore, but in the meantime I tried my best o update to match the new schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
pnpm
as canonical package manager. Dependency requirements are a bit different withpnpm
compared toyarn
, and all package versions have been updated accordingly to comply.corepack
for package manager version control; version maintained inpackage.json
(Enable withcorepack enable
)yarn
and updated topnpm
instructions