-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
[docs-infra] Improve export into sandbox package.json #46044
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
[docs-infra] Improve export into sandbox package.json #46044
Conversation
Netlify deploy previewhttps://deploy-preview-46044--material-ui.netlify.app/ Bundle size report |
private: true, | ||
description, |
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.
Because people might eject out of StackBlitz, the description we add to the StackBlitz, seems also relevant in the package.json.
@@ -64,8 +65,9 @@ export default defineConfig({ | |||
'index.html': CRA.getHtml({ ...demoData, main: `/src/index.${ext}` }), | |||
'package.json': JSON.stringify( | |||
{ | |||
name: 'mui-demo', | |||
version: process.env.LIB_VERSION, |
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.
Because we set "latest" for the dependencies version, and most people who create reproduction never pin the npm packages version, having the versions set at the root should help us reproduce 2+ years old bugs when we try to iterate on some logic.
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.
That's not very useful to recreate the installed environment. First, our packages follow independent versioning so it can't be captured in a single version number. And second, it won't capture transitive dependencies. i.e. you may fix the @mui/material version to 7.3.6, its @mui/utils dependency is defined with caret range, so it may as well install 7.56.12 of that package. Or the bug may have been in vite.
There will be a lockfile in the sandbox of the reproduction which should be sufficient to recreate and inspect the conditions of the bug.
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.
That's not very useful to recreate the installed environment
@Janpot Oh yeah, agree, it only helps to know it's v4 and not v5 or v6 😆.
There will be a lockfile in the sandbox of the reproduction which should be sufficient to recreate and inspect the conditions of the bug.
In the reproduction provided in https://github.com/mui/material-ui/issues, none seems to have this.
(A side note, they all seem to prefer CodeSandbox even when we push to StackBlitz by default in the instructions, maybe some SEO lag, 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.
it only helps to know it's v4 and not v5 or v6
Wouldn't it make more sense then to just replace "latest" with ""?
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.
Yeah, I think it would be better, but this is more time-consuming to implement. I think we can narrow the scope down, make the PR simpler. I'm reverting the version.
@@ -56,7 +56,6 @@ describe('StackBlitz', () => { | |||
</body> | |||
</html>`, | |||
'package.json': `{ | |||
"name": "mui-demo", |
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.
No purpose?
...(templateData.codeVariant === 'TS' && { | ||
main: 'index.tsx', | ||
}), |
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.
Dead logic?
version: process.env.LIB_VERSION, | ||
private: true, |
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.
Having it private sounds wise. Avoid spam on npm.
@@ -262,6 +260,8 @@ ReactDOM.createRoot(document.querySelector("#root")${type}).render( | |||
|
|||
files['package.json'] = { | |||
content: { | |||
version: process.env.LIB_VERSION, |
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.
Same as for StackBlitz
7454943
to
c6cbb07
Compare
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.
Packages without a version are not publishable so the private field doesn't really have any effect anymore. I don't have a problem with keeping the version, if it can help anyone debug. I just don't see it being useful for myself.
From #46034, I noticed that we could sync the CodeSandbox and StackBlitz experience, pick the best of the two worlds.
A continuation of #45924.
Preview: https://deploy-preview-46044--material-ui.netlify.app/material-ui/react-alert/#introduction