Transform codebase to typescript #462

radek-stary · 2023-04-17T21:47:26Z

tsconfig.json

mbohal · 2023-04-19T15:58:34Z

jest.config.js

@@ -2,19 +2,25 @@ module.exports = {
  moduleFileExtensions: [
    'js',


Do we need to keep the jsx extension? Do we want to keep it?

If changing here, please also adjust transform below.

mbohal · 2023-04-19T16:43:46Z

src/docs/_components/Icon/Icon.tsx

Apparently git does not detect the jsx -> tsx rename as a rename.

According to https://stackoverflow.com/questions/29822823/how-does-git-know-that-file-was-renamed this can happen.

Would it be possible to dissect this PR into two commits:

that does the jsx -> tsx rename on files where the rename was nopt detected

another that does the code change

Then, before merge we would squash these two commits. I can not think of another way how to handle this safely and sanely.

With git rebase it should not be too much work.

Apparently git does not detect the jsx -> tsx rename as a rename

My feeling is it depends on the amount/nature of changes inside the renamed file. Sometimes it gets it right, sometimes not (see #440).

mbohal · 2023-04-19T16:46:50Z

tests/propTests_in_javacript/actionColorPropTest.ts

I think the name of the folder holding this file is wrong.

mbohal · 2023-04-19T16:49:24Z

webpack.config.js

@@ -69,7 +74,7 @@ module.exports = (env, argv) => ({
      ? '[name].js'
      : '[name].development.js',
    libraryTarget: 'umd',
-    path: path.join(__dirname, 'dist'),
+     path: path.join(__dirname, 'dist'),


please fix whitespace

Why did ESlint miss that?

mbohal · 2023-04-19T16:50:01Z

webpack.config.js

@@ -2,16 +2,16 @@ const path = require('path');
 const StyleLintPlugin = require('stylelint-webpack-plugin');
 const VisualizerPlugin = require('webpack-visualizer-plugin');

-const MAX_DEVELOPMENT_OUTPUT_SIZE = 3000000;
-const MAX_PRODUCTION_OUTPUT_SIZE = 335000;
+const MAX_DEVELOPMENT_OUTPUT_SIZE = 4000000;


Do we have any ideas why the size increase? Is it really that much (~30%)?

mbohal · 2023-04-19T16:52:11Z

src/lib/translations/en.ts

@@ -9,4 +9,4 @@ export default {
    next: 'Next',
    previous: 'Previous',
  },
-};
+} as Record<string, Record<string, string>>;


I would prefer to create a dedicated interface or type for translation that whitelist that specifies the concrete object keys. That way if anyone needs to provide translations for another language, they can leverage the type system.

mbohal · 2023-04-19T17:42:33Z

tests/providerTests/formLayoutProviderTest.tsx

For some reason this test fails when run as npm run lib:jest. However it passes when run as npm run lib:jest.

The same thing happens in master, but it does not happen in CI.
I have no idea what is going on 😕

@mbohal Both your commands are identical 🙂.

adamkudrna

I'm so curious about the final result, can't wait to finally learn TS 🙂.

To me it appears ESlint is sleeping instead of doing its job… There are many formating issues lost in translation…
Please fix the branch name and commit message according to our conventions so the changes make their way to the CHANGELOG (among other reasons 🙂).

adamkudrna · 2023-04-26T16:09:06Z

.eslintrc

-  }
-}
+  ]
+}


Please fix the new line at the end of the file.

adamkudrna · 2023-04-26T16:12:30Z

definitions.d.ts

+/**
+ * [Color variant](/foundation/colors#component-colors)
+ */
+declare type Color = 'primary' | 'secondary' | 'selected' | 'success' | 'warning' | 'danger' | 'help' | 'info' | 'note' | 'light' | 'dark';


⚠️ Not all colors can be always used. There are multiple "dictionaries" that group colors suitable for certain use case, see RUI docs.

Does it make sense to separate them in TS? (I think it does, but maybe not now.)

Does it make sense to separate them in TS?

Yes, it makes sense to do it now.

adamkudrna · 2023-04-26T16:13:59Z

definitions.d.ts

+
+declare type PredefinedLabelWidthValues = 'auto' | 'default' | 'limited';
+
+// Test types


Why a different type of comment? I mean // vs. /* */.

Secondly, why only some types have comments and some don't?

adamkudrna · 2023-04-26T16:15:28Z

jest.config.js

@@ -2,19 +2,25 @@ module.exports = {
  moduleFileExtensions: [
    'js',


If changing here, please also adjust transform below.

adamkudrna · 2023-04-26T16:15:56Z

package.json

@@ -33,7 +33,8 @@
    "npm": ">=8.3.0"
  },
  "dependencies": {
-    "normalize.css": "^8.0.1"
+    "normalize.css": "^8.0.1",
+    "ts-jest": "^27.1.5"


Isn't it a dev dependency?

adamkudrna · 2023-04-26T16:26:56Z

tests/propTests/sizePropTest.ts

+    { size: 'large' },
+    (rootElement: HTMLElement) => expect(rootElement).toHaveClass('isRootSizeLarge'),
+  ],
+


Needless blank line

adamkudrna · 2023-04-26T16:31:07Z

tests/propTests_in_javacript/actionColorPropTest.ts

I suppose the propTests_in_javascript directory is not going to be merged into master, right?

adamkudrna · 2023-04-26T16:34:56Z

.eslintrc

+    "airbnb-typescript",
+    "plugin:@typescript-eslint/recommended",
+    "plugin:typescript-sort-keys/recommended",
+    // "plugin:@typescript-eslint/recommended-requiring-type-checking"


Is this line going to be used in a next step? If not, I'd delete it.

adamkudrna · 2023-04-26T16:36:40Z

src/docs/_components/Placeholder/Placeholder.tsx

+    </div>
+  );
+};
+export default Placeholder;


There should be a blank line before the default export, I believe.

adamkudrna · 2023-04-26T16:37:31Z

src/lib/components/Alert/Alert.tsx

+
+export const AlertWithGlobalProps = withGlobalProps(Alert, 'Alert');
+
+export default AlertWithGlobalProps;


Please fix the missing end of line

github-actions bot assigned radek-stary Apr 17, 2023

mbohal requested changes Apr 19, 2023

View reviewed changes

radek-stary force-pushed the transform-typescript branch 3 times, most recently from 33dc915 to 73ea1ac Compare April 26, 2023 11:49

adamkudrna reviewed Apr 26, 2023

View reviewed changes

radek-stary force-pushed the transform-typescript branch 5 times, most recently from 516cbc4 to e8cf786 Compare April 28, 2023 02:42

radek-stary changed the title ~~transform codebase to typescript~~ Transform codebase to typescript Apr 28, 2023

radek-stary marked this pull request as draft April 28, 2023 04:42

radek-stary closed this May 1, 2023

radek-stary force-pushed the transform-typescript branch from e8cf786 to 6213b0a Compare May 1, 2023 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transform codebase to typescript #462

Transform codebase to typescript #462

radek-stary commented Apr 17, 2023 •

edited by adamkudrna

Loading

mbohal Apr 19, 2023

adamkudrna Apr 26, 2023

mbohal Apr 19, 2023

adamkudrna Apr 26, 2023 •

edited

Loading

mbohal Apr 19, 2023

mbohal Apr 19, 2023

adamkudrna Apr 26, 2023

mbohal Apr 19, 2023

mbohal Apr 19, 2023

mbohal Apr 19, 2023

adamkudrna Apr 26, 2023

adamkudrna left a comment

adamkudrna Apr 26, 2023

adamkudrna Apr 26, 2023

mbohal May 2, 2023

adamkudrna Apr 26, 2023

adamkudrna Apr 26, 2023

adamkudrna Apr 26, 2023

adamkudrna Apr 26, 2023

adamkudrna Apr 26, 2023

adamkudrna Apr 26, 2023

adamkudrna Apr 26, 2023

adamkudrna Apr 26, 2023

		@@ -2,19 +2,25 @@ module.exports = {
		moduleFileExtensions: [
		'js',


		declare type PredefinedLabelWidthValues = 'auto' \| 'default' \| 'limited';

		// Test types


		export const AlertWithGlobalProps = withGlobalProps(Alert, 'Alert');

		export default AlertWithGlobalProps;

Transform codebase to typescript #462

Transform codebase to typescript #462

Conversation

radek-stary commented Apr 17, 2023 • edited by adamkudrna Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamkudrna Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamkudrna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radek-stary commented Apr 17, 2023 •

edited by adamkudrna

Loading

adamkudrna Apr 26, 2023 •

edited

Loading