[code-infra] Fix deploy #212
Conversation
oliviertassinari
force-pushed
the
fix-contributor-dashboard-legacy
branch
from
cd684ea to
fb3e68c
Compare
oliviertassinari
force-pushed
the
fix-contributor-dashboard-legacy
branch
5 times, most recently
from
12c87cf to
2706b80
Compare
oliviertassinari
force-pushed
the
fix-contributor-dashboard-legacy
branch
5 times, most recently
from
2b38670 to
c1f8eef
Compare
| .cache | ||
| /.git | ||
| /contributor-dashboard-legacy/ | ||
| /tools-public/toolpad/.generated/ | ||
| /tools-public/toolpad/**/*.yml | ||
| build | ||
| node_modules | ||
| pnpm-lock.yaml |
There was a problem hiding this comment.
Get to lint this a bit more.
| { | ||
| files: ['contributor-dashboard-legacy/**'], | ||
| rules: { | ||
| 'import/no-unresolved': 'off', // TODO, to fix at one point |
There was a problem hiding this comment.
This is because we are not using the pnpm workspace for contributor-dashboard-legacy.
One thing that I didn't try is to simply add contributor-dashboard-legacy in the pnpm workspace. Maybe one day in the future.
| /.eslintcache | ||
| /coverage | ||
|
|
||
| # production | ||
| /build | ||
|
|
||
| src |
There was a problem hiding this comment.
That was crazy.
| /tools-public/toolpad/**/*.yml | ||
| /tools-public/toolpad/.generated/ |
There was a problem hiding this comment.
Match mui-private
| * @param {*} event | ||
| * @param {*} context | ||
| */ | ||
| exports.handler = async function circleCIArtefact(event) { |
There was a problem hiding this comment.
Bypass CORS after CircleCI API change.
There was a problem hiding this comment.
Do I understand correctly that to fill the table we're fetching from multiple functions and reconcile them client-side?
It feels somewhat wasteful to me, especially since some of the functions seem to do cache control and others don't. Isn't it more logical to fetch and reconcile data in a single netlify function, add cache control and serve it in a format that can easily be fed into a table?
There was a problem hiding this comment.
🤔 Actually, we're already calculating the diff in the dangerfile. Would probably make more sense to just calculate the diff in CI, store it on file and push it to S3 and make it public. Then danger can just read the file and the UI/toolpad can fetch from S3. We wouldn't have to fetch and calculate in 3 places and we wouldn't have the problem with disappearing expired build artifacts neither.
Not for this PR though.
| "prettier": "^2.0.5", | ||
| "pretty-quick": "^3.1.3", | ||
| "prettier": "^3.3.3", | ||
| "pretty-quick": "^4.0.0", |
There was a problem hiding this comment.
Sync with monorepo standard
I'd imagine over time we steer away from Studio for this use case. I feel like the only reason it's implemented in Toolpad is to serve as dog-fooding. But the people maintaining it are not really the target audience. I'd rather imagine us to migrate the existing code to the X data grid instead to reduce code size and improve UX. I'm not sure i understand the search support comment. Couldn't you filter the datagrid? The old netlify version doesn't have search support, right? |
| @@ -62,7 +63,11 @@ const Ribbon = styled.a` | |||
|
|
|||
| /* Set the text properties */ | |||
| color: #fff; | |||
| font: 700 1em "Helvetica Neue", Helvetica, Arial, sans-serif; | |||
| font: | |||
There was a problem hiding this comment.
interesting, tabs vs spacing everywhere. I would imagine prettier complaining about this
There was a problem hiding this comment.
Prettier made this change. I didn't know it had support for styled components, but looking at issues like prettier/prettier#2291, it looks like so.
There was a problem hiding this comment.
Didn't mean this line specifically, not sure why I put the comment here. I'm just surprised to see tabs everywhere. Doesn't prettier indent with spaces by default?
There was a problem hiding this comment.
I'd imagine over time we steer away from Studio for this use case. I feel like the only reason it's implemented in Toolpad is to serve as dog-fooding. But the people maintaining it are not really the target audience. I'd rather imagine us to migrate the existing code to the X data grid instead to reduce code size and improve UX.
@Janpot Ok, could be, I don't have specific views on this.
I'm not sure i understand the search support comment. Couldn't you filter the datagrid? The old netlify version doesn't have search support, right?
Sorted by UX experience:
- Borderline: Current https://tools-public.mui.com/prod/pages/bundleSizes
- Good: Support for data grid quick-filter would be good. I could manage with this. The problem is that I would likely want to search but still see rows around matches for the context
- Great: The benchmarks used in [data grid] Implement Search (equivalent Ctrl + F) mui-x#5082
- Amazing: Hook into the native search of the browser (not possible today, see spec issue linked in the previous issue)
| @@ -62,7 +63,11 @@ const Ribbon = styled.a` | |||
|
|
|||
| /* Set the text properties */ | |||
| color: #fff; | |||
| font: 700 1em "Helvetica Neue", Helvetica, Arial, sans-serif; | |||
| font: | |||
There was a problem hiding this comment.
Prettier made this change. I didn't know it had support for styled components, but looking at issues like prettier/prettier#2291, it looks like so.
Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Olivier Tassinari <[email protected]>
Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Olivier Tassinari <[email protected]>
Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Olivier Tassinari <[email protected]>
oliviertassinari
force-pushed
the
fix-contributor-dashboard-legacy
branch
from
de8cf31 to
0e52db9
Compare
| ); | ||
| downloadURL.searchParams.set("url", artifact!.url); | ||
| downloadURL.searchParams.set("url", sizeSnapshotArtifact!.url); |
There was a problem hiding this comment.
Unless we want to shave off every byte possible from the bundle, I'd throw a nice error instead of the cryptic "Cannot read properties of undefined (reading 'url')".
There was a problem hiding this comment.
Agree, bundle size is a negligible concern for internal tools, developer speed is more important.
Now contributor-dashboard-legacy was written quick and dirty by Sebastian, optimizing for speed, like I did with https://mui.com/store/, and now, Vadym is cleaning that up progressively. I think we should be super lazy for internal tools, and only fix the stuff that we felt as painful.
A quick fix for something that I noticed we regressed on from mui/material-ui#44075.
The problems:
I have tried to do the least amount of work possible. My thought is that we are likely to use more Netlify and Toolpad Studio in tandem as we move forward, so we likely want both to work. We also need to have a fallback for Toolpad as bundle size tracker feels like a critical tool, so if Toolpad Studio go away, we would need this, and last, I wasn't clear on how far we migrated the tool to Toolpad, I was wonder if we were missing anything.
So far, the only miss I see to fully migrate from this custom <MaterialUI.Table> to Toolpad Studio is search support, it's painful without it.
After: https://deploy-preview-212--mui-dashboard.netlify.app/size-comparison?circleCIBuildNumber=759483&baseRef=master&baseCommit=a25a365a4c66738f358ecd745bf3727c0ca9d5b6&prNumber=44075