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

Upgrade UI deps #3163

Merged
merged 12 commits into from Jul 30, 2021
Merged

Upgrade UI deps #3163

merged 12 commits into from Jul 30, 2021

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Jul 19, 2021

Description of the change

This PR simply upgrades the UI deps, moves (and cleans up) the types deps to devOnly ones and performs minor changes related to the types (ie adding missing fields)

Benefits

We will be able to safely upgrade our UI deps again.

Possible drawbacks

Like in any other upgrade, perhaps we are missing something hidden behind the unit, e2e and manual tests.

Applicable issues

Additional information

Thanks @dlaloue-vmware for jumping in and suggesting the isomorphic-form-data dep.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	dashboard/yarn.lock
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Comment on lines -13 to -30
"@types/diff": "^5.0.1",
"@types/google-protobuf": "^3.15.3",
"@types/js-yaml": "^4.0.2",
"@types/json-schema": "^7.0.8",
"@types/jsonwebtoken": "^8.5.4",
"@types/lodash": "^4.14.171",
"@types/moxios": "^0.4.12",
"@types/pako": "^1.0.2",
"@types/qs": "^6.9.7",
"@types/react": "^17.0.15",
"@types/react-copy-to-clipboard": "^5.0.1",
"@types/react-jsonschema-form": "^1.7.6",
"@types/react-router-dom": "^5.1.8",
"@types/react-router-hash-link": "^2.4.1",
"@types/react-tabs": "^2.3.3",
"@types/react-transition-group": "^4.4.2",
"@types/semver": "^7.3.8",
"@types/ws": "^7.4.7",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to dev deps. Also, I removed all of them and just installed those ones Typescript was complaining (this way we reduce the types dependencies (as many of them are including also the definition of the types)

Comment on lines +61 to +63
<MemoryRouter initialEntries={["/foo?q=foo"]}>
<AppList />
</MemoryRouter>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main change here is replacing the jest.spyOn with the approach suggested in the official website: https://reactrouter.com/web/guides/testing

state: "",
} as any);
const wrapper = mountWrapper(defaultStore, <ContextSelector />);
const history = createMemoryHistory({ initialEntries: ["/c/default-cluster/ns/ns-bar/catalog"] });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative way to the which allows using the variable to perform the checks

@@ -3,6 +3,7 @@ import { shallow } from "enzyme";
import Checkbox from ".";

const defaultProps = {
id: "test",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A test that was generating an annoying warning for long time ago :P

import configureMockStore from "redux-mock-store";
import thunk from "redux-thunk";
import AppUpgrade from "../../components/AppUpgrade";
import Upgrade from "./AppUpgradeContainer";

const mockStore = configureMockStore([thunk]);

const emptyLocation = {
const emptyLocation: Location = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just adding the type here (as in other files) and adding the required fields we were missing.

Comment on lines +84 to +91
return (
<Redirect
to={{ pathname: app.apps.list(this.props.cluster, this.props.currentNamespace) }}
/>
);
}
// There is not a default namespace, redirect to login page
return <Redirect to={"/login"} />;
return <Redirect to={{ pathname: "/login" }} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After upgrading the types, to has a required pathname property we were missing. I've updated the tests accordingly.

@antgamdia antgamdia marked this pull request as ready for review July 29, 2021 21:17
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Great - one small comment about the new dep (which may not be required soon).

@@ -37,6 +19,7 @@
"google-protobuf": "^3.17.3",
"history": "^4.10.1",
"immutable": "^4.0.0-rc.14",
"isomorphic-form-data": "^2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

So it sounds from @dlaloue-vmware 's comments that this dependency is required by swagger-ui-react but wasn't declared as a dependency by them? Or something similar: swagger-api/swagger-ui#7436

Hah, although they mention there that: "On Thursday 29.7.2021 there will a new release of swagger-ui-react that will fix this issue automatically by updating to it.". Not sure if we wait, but if not, let's at least include a #TODO to remove it refering to the swagger-ui issue ?

Copy link
Contributor Author

@antgamdia antgamdia Jul 30, 2021

Choose a reason for hiding this comment

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

While writing this message, they have just bumped up the version: https://github.com/swagger-api/swagger-ui/releases/tag/v3.51.2, so I have updated it and removed the unnecessary dep straight away. Thanks for the pointer!

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	dashboard/package.json
	dashboard/yarn.lock
@antgamdia antgamdia merged commit 9e5a592 into vmware-tanzu:master Jul 30, 2021
@antgamdia antgamdia deleted the updateUIDeps branch July 30, 2021 11:57
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.

Failing tests after upgrading UI dependencies
2 participants