-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Improve packages exports #10995
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
base: next
Are you sure you want to change the base?
Improve packages exports #10995
Conversation
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.
Other than that, LGTM! I could not find a project setup where this does not work, so congrats!
@@ -1,4 +1,4 @@ | |||
import get from 'lodash/get'; | |||
import get from 'lodash/get.js'; |
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.
nitpick: you could update the message in the custom ESLint rule here:
Line 89 in bf68d2c
"Named import from lodash should be avoided for performance reasons. Use a default import instead. E.g. `import merge from 'lodash/merge';` instead of `import { merge } from 'lodash';`.", |
import { FormGroupsProvider } from './groups/FormGroupsProvider'; | ||
import { getSimpleValidationResolver, type ValidateForm } from './validation'; |
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.
noob question: why that change?
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 try to avoid importing stuff from barrel files. This improves performances for many tools such as Jest. I update the files I work on when I can
@@ -1,3 +1,4 @@ | |||
import type {} from '@mui/material/themeCssVarsAugmentation'; |
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 don't understand this line. Can you explain?
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.
We use to have this in a .d.ts
file but it causes issues with zshy
. As for why we have the line itself, see https://mui.com/material-ui/customization/css-theme-variables/usage/#typescript
Problem
Our packages exports are wrong, making them unusable in Node contexts (for instance when using SSR or RSC frameworks)
Solution
.js
extensions to ourlodash
imports as it causes issues in ESM setupHow To Test
make build
npm pack
in each package you'll want to use in the tests explained below. You'll have to move/copy them in avendor
directory and provide their path in your test applicationpackage.json
.ra-core
,ra-data-fakerest
,ra-i18n-polyglot
,ra-language-english
anddata-generator-retail
ESM
shadcn-admin-kit
vendor
directory:/admin
route:CJS
node-test
directorydata-generator-retail
from thevendor
directory as explained beforeindex.cjs
(note the .cjs extension) file with the following content:node ./index.cjs
Additional Checks
master
for a bugfix or a documentation fix, ornext
for a feature