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

Enable npm workspaces #3765

Merged
merged 9 commits into from
Jul 14, 2023
Merged

Enable npm workspaces #3765

merged 9 commits into from
Jul 14, 2023

Conversation

zxbodya
Copy link
Contributor

@zxbodya zxbodya commented Jul 13, 2023

Reasons for making this change

Enabling workspaces, to improve dependency installation time and IDE performance/usability (having much less code to index making things faster)

Similar to #3382 - which btw would be great when ready, and probably can be considered intermediate step on the way to it.

By using npm workspaces, it is possible to get good improvements, while not having to fix symlink compatibility issues (which I guess is what is yet to be looked into for using pnpm)

@@ -6,7 +6,5 @@ module.exports = {
},
moduleNameMapper: {
'\\.(css|less)$': '<rootDir>/__mocks__/styleMock.js',
'^react$': '<rootDir>/node_modules/react',
'^react-dom$': '<rootDir>/node_modules/react-dom',
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 no longer correct or needed, instead there is single react version which is resolved correctly

@@ -14176,7 +14176,8 @@ label[data-shrink=false]+.MuiInputBase-formControl .emotion-3:focus::-ms-input-p
"height": 0,
"left": 0,
"overflow": "hidden",
"padding": 0,
"paddingBottom": 0,
"paddingTop": 0,
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 because not all package-lock files were used, and effectively an update for some of the dependencies in repository (change is coming from the new mui version)

'@emotion/react': path.resolve('./node_modules/@emotion/react'),
'@emotion/styled': path.resolve('./node_modules/@emotion/styled'),
},
alias: {},
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 to be incorrect path (this dependencies happen to be installed in top level node_modules), also everything still works as expected without this

@@ -46,7 +46,6 @@
"@babel/plugin-transform-react-jsx": "^7.22.5",
"@babel/preset-env": "^7.22.5",
"@babel/preset-react": "^7.22.5",
"@types/jest-expect-message": "^1.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

types are loaded from jest-expect-message directly

@@ -34,6 +33,7 @@
"devDependencies": {
"@babel/eslint-parser": "^7.22.5",
"@nrwl/nx-cloud": "^15.3.5",
"@types/estree": "^1.0.1",
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 to avoid typescript errors from @types/eslint package for which outdated @types/estree peer dependency will be installed otherwise

better fix, maybe as a next step would be to update dts-cli

@heath-freenome
Copy link
Member

@zxbodya I'm not sure why these changes are not working on the github actions. Can you investigate?

@zxbodya
Copy link
Contributor Author

zxbodya commented Jul 13, 2023

@zxbodya I'm not sure why these changes are not working on the github actions. Can you investigate?

yes, looking into it - it looks to be because of cache-dependency-path being misconfigured (just pushed a fix)

@heath-freenome
Copy link
Member

Also, I'm releasing 5.10.0 right now which includes bumping packages so once I merge it I expect you will have to resolve conflicts

@heath-freenome
Copy link
Member

Ok, resolve those merge commits...

@heath-freenome heath-freenome merged commit ea36b6e into rjsf-team:main Jul 14, 2023
4 checks passed
@zxbodya zxbodya deleted the npm-workspaces branch July 14, 2023 15:16
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.

2 participants