-
Notifications
You must be signed in to change notification settings - Fork 4
Prep Repo for Tldraw Upgrade #317
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
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
e9d1913 to
dff8ad8
Compare
|
LGTM. No error on Now, a slight concern going forward : website now uses react 19, roam uses react 18, so does it even make sense to have a packages/ui, since no ui code can be common? |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Just noting that we discussed this in our Dev meeting. |
|
@mdroidian there are some errors on my trial. Here's what i did:
Update: I also tried the other method of cloning to a separate test folder and received the same errors The errors:
|
|
so i just But I realized the build error was because of a nanoid import, so after resolving that build was successful. However, the apps/obsidian folder still doesn't have node_modules within it. |
| }, | ||
| "roamjs-components": { | ||
| "react": "18.2.0", | ||
| "react-dom": "18.2.0" |
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.
I ran into an issue with conflicts in @types/react, @types/react-dom. I think those should be added.
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.
@@ -36,19 +36,27 @@
"overrides": {
"@blueprintjs/core": {
"react": "18.2.0",
- "react-dom": "18.2.0"
+ "react-dom": "18.2.0",
+ "@types/react": "18.2.0",
+ "@types/react-dom": "18.2.0"
},
"@blueprintjs/datetime": {
"react": "18.2.0",
- "react-dom": "18.2.0"
+ "react-dom": "18.2.0",
+ "@types/react": "18.2.0",
+ "@types/react-dom": "18.2.0"
},
"@blueprintjs/select": {
"react": "18.2.0",
- "react-dom": "18.2.0"
+ "react-dom": "18.2.0",
+ "@types/react": "18.2.0",
+ "@types/react-dom": "18.2.0"
},
"roamjs-components": {
"react": "18.2.0",
- "react-dom": "18.2.0"
+ "react-dom": "18.2.0",
+ "@types/react": "18.2.0",
+ "@types/react-dom": "18.2.0"
}
}
}
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.
Can you go into more detail? What do you mean by "ran into conflicts"? Is this new behavior since the last time you ran npm i? Or something else?
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.
@maparent roamjs-components version 0.85.2 removes the potential offending package.
EDIT, scratch that, 0.85.3
This should be fine. |


This will be a draft PR that is broken down into multiple smaller PRs with the goal of preparing the repo for
apps/roamtldraw upgrade and to help move our repo back to a healthier state.In this PR, the following changes have been made:
apps/roampackage-lock.jsonRemove package-lock.json in apps/roam #70apps/roamto React 18roamjs-componentsandblueprintjsapps/website/package/uito React 19--legacy-peer-deps( rmnode_modules, rmpackage-lock,npm ito rebuildpackage-lock)node_modulespostinstallto apply root packagesCaveats
children, this will show a type error currentlynpm ls reactstill shows many errors, mostly from blueprintjs. We may fork blueprintjs in the future to solve this.packages/uito React 19.apps/roamwill not usepackages/ui.packages/uitopackages/utilso they can be used acrossapps/*(ENG-633)Testing
Setup
To test this with your app/workflow, you can either clone to test folder or remove
node_modules:git clonethe repo to a new temp foldereng-617-reproduce-the-idenpm-issuesOR
node_modules(from root and eachapp/*andpackage/*if they exist)THEN
package-lock.jsonnpm iTest
Let me know if after
npm ipackage-lock.jsonshows changes from this PR'spackage-lock.jsonSummary by CodeRabbit
New Features
Refactor
Chores