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

Fix up some config etc. (attempt 2) #1018

Merged
merged 18 commits into from
Oct 18, 2023
Merged

Fix up some config etc. (attempt 2) #1018

merged 18 commits into from
Oct 18, 2023

Conversation

webpro
Copy link
Contributor

@webpro webpro commented Oct 10, 2023

This PR improves/fixes some configuration and dependencies in our monorepo.

(Duplicating #1008, as CI runs kept getting cancelled (devops cancel culture?). Eventually traced it down to a combo of adding the root workspace (- '.') to pnpm-workspace.yaml and "extends": ["//"] to the root turbo.json (which turbo documents to do). Oh well, let's move on.)

Here's the list of commits:

  • Fix pretter/prettier-plugin-packagejson location (77d18c7)
  • Sync versions across workspaces (dcf49b6)
  • Fix turbo config to include root workspace (4e16c14)
  • Temp fix for knip + next-translate-plugin issue (64eb4dc)
  • Upgrade some deps (e717af6)
  • Update changelog (e33b174)
  • Trial & error: upgrade @microsoft/api-extractor (80907c0)
  • Trial & error: abandon root workspace idea in pnpm-workspace.yaml (301ce96)
  • Add linter + fix issues in root workspace (7eede98)

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2023

🦋 Changeset detected

Latest commit: 31ce1df

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Oct 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
alpha-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 17, 2023 8:34pm
docs-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 17, 2023 8:34pm
tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 17, 2023 8:34pm
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
immutable-records ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2023 8:34pm
react-ui ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2023 8:34pm

@webpro webpro force-pushed the chore/fix-up-config branch 2 times, most recently from 7eede98 to ff2f205 Compare October 10, 2023 09:03
@webpro webpro marked this pull request as ready for review October 10, 2023 09:36
@@ -0,0 +1,27 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need 2 changesets?

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 think it's coming from the fact I branched from another feat branch. I'll keep it in mind. Either way, it's all none so it'll basically end up nowheere.

@@ -76,16 +76,15 @@
"@vanilla-extract/jest-transform": "1.1.1",
"@vanilla-extract/next-plugin": "2.3.1",
"@walletconnect/types": "~2.8.1",
"@webpro/next-translate-plugin": "^2.6.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tl/dr; that plugin doesn't support custom base path cq monorepos so I fixed it. We'll go back to the original once it's sorted downstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a section in the readme or amend this commit to add this PR in the message? It's easier to find why we have this in transition for the future

@@ -57,7 +57,7 @@
"mdast-util-to-markdown": "~1.5.0",
"mdast-util-to-string": "~3.2.0",
"mobx": "~6.9.0",
"next": "13.5.3",
"next": "13.4.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason for downgrading next version here?

cc @sstraatemans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a policy in the repo to make sure the version of a package is the same across the monorepo. First I tried to upgrade the others, but got some error, so then downgraded this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm the new version was helping us with some performance.
not really happy with this one

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense for packages like next to have an exception, this is something close to the app itself, you don't want to force the whole repo to upgrade it at once every time we upgrade in the future. Already proven by the fact that you had troubles doing it now.

next isn't some bundled dependency that is going to give issues in builds or increase bundle sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next.js has a dependency on React. Component libs too. So to prevent any issues here (we've had them before) we should stay in sync, especially libs like Next.js + React.

@@ -110,8 +110,7 @@
"micromark-extension-frontmatter": "~1.1.0",
"mini-css-extract-plugin": "2.7.6",
"openapi-types": "~12.1.3",
"prettier": "~3.0.0",
"prettier-plugin-packagejson": "^2.4.5",
"prettier": "~3.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

if we're allowing prettier at root level dependency, then why are we keeping prettier again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To keep them as much self-contained/isolated as possible.

@webpro webpro force-pushed the chore/fix-up-config branch from 5907e98 to 930ff26 Compare October 17, 2023 17:54
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.

7 participants