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.

Sign up for GitHub

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

Jump to bottom

Replace react-scripts-ts with official react-scripts #1823

Merged
merged 6 commits into from
Jun 25, 2020
Merged

Replace react-scripts-ts with official react-scripts #1823

merged 6 commits into from
Jun 25, 2020

Conversation

andresmgot
Copy link
Contributor

Description of the change

The official react-scripts package supports TS so we don't need to keep using the outdated react-script-ts. I based the migration in the steps described here: https://joshblog.net/2018/upgrading-a-react-and-typescript-app-from-react-scripts-ts-to-create-react-app-v2-1/. For that I had to:

  • Upgrade some dependencies.
  • Update the package.json and tsconfig.json with some new properties/config options.
  • Fix a ton of linter/syntax errors.
  • Fix associated tests.

Applicable issues

absoludity
absoludity approved these changes Jun 25, 2020
View reviewed changes
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.

Wow - great work Andres. A bunch of comments mainly to do with any (which you mentioned was just temporary) and some other irregularities that aren't obvious. But we can deal with those in a followup if needed.

dashboard/package.json
"last 1 chrome version",
"last 1 firefox version",
"last 1 safari version"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks interesting!? https://create-react-app.dev/docs/supported-browsers-features/#configuring-supported-browsers

dashboard/src/components/AppUpgrade/AppUpgrade.tsx
@@ -9,14 +9,14 @@ import SelectRepoForm from "../SelectRepoForm";
import UpgradeForm from "../UpgradeForm";

export interface IAppUpgradeProps {
app: IRelease;
app?: IRelease;
Copy link
Contributor

Choose a reason for hiding this comment

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

Eek, why are these changing to optional props? (and then the bangs below saying they're always defined)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are undefined while the call to getAppWithUpdateInfo is being executed, we can avoid the bangs though

dashboard/src/components/AppUpgrade/AppUpgrade.tsx Outdated
@@ -27,7 +27,7 @@ export interface IAppUpgradeProps {
values?: string,
schema?: JSONSchema4,
) => Promise<boolean>;
fetchChartVersions: (namespace: string, id: string) => Promise<IChartVersion[]>;
fetchChartVersions: (namespace: string, id: string) => Promise<any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interested to test why this is needed too (though I trust that it is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, actually is not any, but it could be undefined

dashboard/src/components/AppView/AppView.test.tsx
@@ -33,7 +33,7 @@ describe("AppViewComponent", () => {
namespace: "weee",
});

const validProps: IAppViewProps = {
const validProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto (marking now so if I land this I can check these points myself).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is because if I let the IAppViewProps, then validProps.app may be undefined (according to ts) but if I don't set the type, validProps.app is always defined and I don't need to change any test.

dashboard/src/components/AppView/AppView.tsx
@@ -19,7 +19,7 @@ import ResourceTable from "./ResourceTable";
export interface IAppViewProps {
namespace: string;
releaseName: string;
app: IRelease;
app?: IRelease;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reason, app will be undefined while fetching

dashboard/src/components/DeploymentFormBody/BasicDeploymentForm/TextParam.tsx Outdated
handleBasicFormParamChange: (
p: IBasicFormParam,
) => (e: React.FormEvent<HTMLInputElement | HTMLTextAreaElement | HTMLSelectElement>) => void;
handleBasicFormParamChange: (param: IBasicFormParam) => (e: React.FormEvent<any>) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Check.

dashboard/src/components/LoadingWrapper/index.ts
import LoadingWrapper, { ILoadingWrapperProps, LoaderType } from "./LoadingWrapper";
export { ILoadingWrapperProps, LoaderType };
import LoadingWrapper, { LoaderType } from "./LoadingWrapper";
export { LoaderType };
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - related to one of my questions above.

dashboard/src/components/OperatorView/OperatorView.tsx Outdated
@@ -19,7 +19,7 @@ interface IOperatorViewProps {
namespace: string;
error?: Error;
push: (location: string) => RouterAction;
getCSV: (namespace: string, name: string) => Promise<IClusterServiceVersion>;
getCSV: (namespace: string, name: string) => Promise<any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Check.

dashboard/src/components/UpgradeForm/UpgradeForm.tsx Outdated
@@ -33,7 +33,7 @@ export interface IUpgradeFormProps {
) => Promise<boolean>;
push: (location: string) => RouterAction;
goBack: () => RouterAction;
fetchChartVersions: (namespace: string, id: string) => Promise<IChartVersion[]>;
fetchChartVersions: (namespace: string, id: string) => Promise<any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Check

dashboard/src/shared/Config.test.ts
@@ -31,7 +31,6 @@ describe("Config", () => {
});

it("does not returns the overriden namespace if NODE_ENV=production", async () => {
process.env.NODE_ENV = "production";
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing there was some other reason for removing this, but it appears to make this contradict the previous one above, yet they both pass?

@absoludity absoludity merged commit 4740a83 into master Jun 25, 2020
@absoludity absoludity deleted the remove-react-script-ts branch June 25, 2020 23:30
@absoludity absoludity mentioned this pull request Jun 26, 2020
Merged
andresmgot pushed a commit that referenced this pull request Jun 26, 2020
@absoludity
Small adjustments to #1823 (#1825)
f49b3ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absoludity absoludity absoludity approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace package react-scripts-ts
2 participants
@andresmgot @absoludity