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.

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

Reconfigured Yarn and package.json with more universal approach #158

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

mkramek
Copy link
Contributor

@mkramek mkramek commented Mar 26, 2024

Summary

  • Reconfigured .yarnrc.yml for universality and better compatibility
  • Added support for engines field in package.json

Ticket ID/Link to ClickUp

Improve .yarnrc.yml configuration

Checklist

  • Provided a screenshot (only if change is visible in UI)
  • Provided a change scope in summary
  • Local Docker build has passed

@hkrawczyk
Copy link
Contributor

@mkramek mkramek enabled auto-merge March 26, 2024 16:05
@ItsAdel
Copy link
Contributor

ItsAdel commented Mar 26, 2024

Seems like we are maintaining another binary file in our repo with this update instead of the yarn4.1.1 binary.

Just don't want the hassle of maintaining binaries to fall on us later down the road. But if the team agrees this is the best approach, then we can just move forward with this change

we have a line "packageManager": "[email protected]" in package.json, is that not enough to enforce the correct yarn is being used?

@hkrawczyk
Copy link
Contributor

Seems like we are maintaining another binary file in our repo with this update instead of the yarn4.1.1 binary.

Just don't want the hassle of maintaining binaries to fall on us later down the road. But if the team agrees this is the best approach, then we can just move forward with this change

we have a line "packageManager": "[email protected]" in package.json, is that not enough to enforce the correct yarn is being used?

I agree with @ItsAdel - let's not add a package manager to the repo, it's a strange practice.

@@ -0,0 +1,11 @@
/* eslint-disable */
// prettier-ignore
// noinspection ES6ConvertVarToLetConst,UnnecessaryLocalVariableJS,CommaExpressionJS
Copy link
Contributor

Choose a reason for hiding this comment

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

please specify the version in package.json rather than get extra files in repo

@mkramek
Copy link
Contributor Author

mkramek commented Mar 26, 2024

Seems like we are maintaining another binary file in our repo with this update instead of the yarn4.1.1 binary.

Just don't want the hassle of maintaining binaries to fall on us later down the road. But if the team agrees this is the best approach, then we can just move forward with this change

we have a line "packageManager": "[email protected]" in package.json, is that not enough to enforce the correct yarn is being used?

This specific binary is used specifically as a plugin, which is also mentioned in .yarnrc.yml under plugins section. The reason for that is Yarn 2+ completely and blatantly ignoring any package.json remarks, as known in this issue (Yarn issues page); they haven't added this feature natively yet, hence the plugin.

@ItsAdel
Copy link
Contributor

ItsAdel commented Mar 26, 2024

This specific binary is used specifically as a plugin, which is also mentioned in .yarnrc.yml under plugins section. The reason for that is Yarn 2+ completely and blatantly ignoring any package.json remarks, as known in this issue (Yarn issues page); they haven't added this feature natively yet, hence the plugin.

My approach would be to leave out the binaries for now until this feature is natively needed. Strict enforcing of yarn 4.1.1 in our package manager is not necessary. Remove all that is causing the build to fail.

In our CI/CD pipeline (Dockerfile), we ensure the correct yarn file is being installed anyways.

# Replace legacy Yarn with modern one
RUN npm uninstall -g yarn
RUN corepack enable
RUN corepack prepare --activate yarn@stable

And in our local environment, our team is small enough to have this just in the documentation somewhere (README) or just be tribal knowledge.

I would steer away from over complicating it.

@mkramek
Copy link
Contributor Author

mkramek commented Mar 26, 2024

I would steer away from over complicating it.

Makes sense, scratched that

@@ -5,6 +5,11 @@
"cacheDirectories": [
".next/cache"
],
"engineStrict": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed anymore I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, f... I didn't see that comment. I guess I'll make a quick fix?

@mkramek mkramek merged commit 361b1f1 into develop Mar 26, 2024
@Kostanios Kostanios deleted the CU-86azuzuv9--Improve-yarnrcyml-configuration branch April 5, 2024 10:57
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.

3 participants