Update nextjs to version 12 and upgrate Vitessce to latest #897
Conversation
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #897 +/- ##
==========================================
- Coverage 86.19% 85.90% -0.30%
==========================================
Files 544 544
Lines 10084 10089 +5
Branches 2125 2126 +1
==========================================
- Hits 8692 8667 -25
- Misses 1342 1371 +29
- Partials 50 51 +1
|
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
ivababukova
changed the title
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
Signed-off-by: ivababukova <[email protected]>
updated docker build-push in CI
| test: /\.(js|jsx)$/, | ||
| exclude: /(node_modules|bower_components)/, | ||
| loader: 'babel-loader', | ||
| options: { presets: ['@babel/preset-env', '@babel/preset-react'] }, |
There was a problem hiding this comment.
You already set the presets in the .babelrc.js file. Do you need to have this configuration here?
There was a problem hiding this comment.
We need to have it. If I comment it out, I get this build error:
| testPathIgnorePatterns: [ | ||
| '.*\\.mock\\.js', | ||
| 'test-utils', | ||
| ], | ||
| transform: { | ||
| '^.+\\.(js|jsx|ts|tsx)$': '<rootDir>/node_modules/babel-jest', | ||
| '^.+\\.css$': '<rootDir>/config/jest/cssTransform.js', | ||
| '^(?!.*\\.(js|jsx|ts|tsx|css|json)$)': '<rootDir>/config/jest/fileTransform.js', |
There was a problem hiding this comment.
So jest doesnt need this transform function anymore? Was this ran before for every file that is not js|jsx|ts|tsx|css|json ?
There was a problem hiding this comment.
Seems like we don't need it anymore. I will also delete fileTransform.js because it doesn't seem to be referenced anywhere else
| @@ -1,181 +1,161 @@ | |||
| import React from 'react'; | |||
| import { mount } from 'enzyme'; | |||
There was a problem hiding this comment.
If we don't have any enzyme tests anymore, can you remove the module?
There was a problem hiding this comment.
I am afraid we still have enzyme tests -- for example ColorPicker.test.jsx, PipelineLoadingScreen.test.jsx
| await act(async () => { | ||
| fireEvent.click(sampleOrGroupOption); | ||
| }); | ||
| userEvent.click(selectSampleOrGroup); |
There was a problem hiding this comment.
Why remove the act. We dont need it anymore?
There was a problem hiding this comment.
userEvent is a library built on top of fireEvent that provides a more user-centric API for triggering events. userEvent automatically wraps all its methods in act(), so you don't need to use act() when using userEvent in most cases. This is one of the reasons why userEvent is recommended for most interaction testing over fireEvent.
In other words, userEvent takes care of using act() internally, ensuring that all state updates and effects are processed when the event is fired, which is why act is not needed anymore in the revised code snippet.
There was a problem hiding this comment.
it's from chatgpt I think it sums it well
There was a problem hiding this comment.
in reality, when this test was failing, I looked at Agi's changes: https://github.com/biomage-org/ui-old/pull/200/files to see how he fixed the test and I copied what he did, which worked.
| @@ -0,0 +1,6 @@ | |||
| // Due to a bug with dynamic imports and webpack, we need to create | |||
| // a wrapper component to import the Scatterplot component. | |||
| // See https://github.com/webpack/webpack/issues/13865 | |||
There was a problem hiding this comment.
Very weird bug. So importing it here in the wrapper works, but not in other places? How?
There was a problem hiding this comment.
Yes, it is super weird. I think that the bug is that you can't both import the real thing (@vitessce/heatmap) and use dynamic imports in one place -- you have to do it one by one, because webpack doesn't like they syntax of it written all at once. This is why it works when you do it with a wrapper. This is my understanding.
| @@ -32,10 +32,9 @@ import { | |||
| } from 'utils/plotUtils'; | |||
| import getContainingCellSetsProperties from 'utils/cellSets/getContainingCellSetsProperties'; | |||
|
|
|||
| const Scatterplot = dynamic( | |||
| () => import('vitessce/dist/umd/production/scatterplot.min').then((mod) => mod.Scatterplot), | |||
There was a problem hiding this comment.
I think if you use the relative path it will work. Can you try?
webpack/webpack#13865 (comment)
There was a problem hiding this comment.
If it works can you do it in the other places (I saw the same for the heatmap import)
There was a problem hiding this comment.
I tried that and it doesn't work.
There was a problem hiding this comment.
Then I recommend you put all the vitessce wrappers in one file so its less confusing
There was a problem hiding this comment.
Good idea! I have done that in my new branch and PR.
| @@ -12,8 +12,7 @@ import EditableField from 'components/EditableField'; | |||
| import ColorPicker from 'components/ColorPicker'; | |||
| import FocusButton from 'components/FocusButton'; | |||
| import HideButton from 'components/data-exploration/cell-sets-tool/HideButton'; | |||
|
|
|||
| import 'components/data-exploration/hierarchical-tree/hierarchicalTree.css'; | |||
| // import styles from '.hierarchicalTree.module.css'; | |||
Signed-off-by: ivababukova <[email protected]>
…t happen anymore. Delete commented out css import Signed-off-by: ivababukova <[email protected]>
… into ivababukova/update-nextjs
📦 Next.js Bundle Analysis for uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 🎉 Global Bundle Size Decreased
DetailsThe global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster. Any third party scripts you have added directly to your app using the If you want further insight into what is behind the changes, give @next/bundle-analyzer a try! Nineteen Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. |
Signed-off-by: ivababukova <[email protected]>
📦 Next.js Bundle Analysis for uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 🎉 Global Bundle Size Decreased
DetailsThe global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster. Any third party scripts you have added directly to your app using the If you want further insight into what is behind the changes, give @next/bundle-analyzer a try! Nineteen Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. |
|
I had to close this PR and reopen a new one, because of some commits not being signed. Here is the new PR: #904 |
Description
Staged link: https://ui-ivababukova-upgrades.scp-staging.biomage.net/
Details
URL to issue
N/A
Link to staging deployment URL (or set N/A)
N/A
Links to any PRs or resources related to this PR
Integration test branch
master
Merge checklist
Your changes will be ready for merging after all of the steps below have been completed.
Code updates
Have best practices and ongoing refactors being observed in this PR
Manual/unit testing
Integration testing
You must check the box below to run integration tests on the latest commit on your PR branch.
Integration tests have to pass before the PR can be merged. Without checking the box, your PR
will not pass the required status checks for merging.
Documentation updates
Optional