Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Typescript #298

Merged
merged 21 commits into from
Oct 3, 2019
Merged

Typescript #298

merged 21 commits into from
Oct 3, 2019

Conversation

vincaslt
Copy link
Contributor

@vincaslt vincaslt commented Sep 23, 2019

Opened this PR to collect feedback and keep track of cleanup stuff.

Related PRs:

react-components(this): #298 4️⃣
proton-pack: https://github.com/ProtonMail/proton-pack/pull/26/files 1️⃣
proton-i18n: ProtonMail/proton-i18n#9 2️⃣
proton-shared: https://github.com/ProtonMail/proton-shared/pull/53/files 3️⃣
protonmail-settings: ProtonMail/proton-mail-settings#131 5️⃣
Implementing typescript support through babel loaders as opposed to webpack loaders. This will use '@babel/preset-typescript' to compile js and ts files.

Caveats:

  1. Babel won't do live typecheck. This is not a problem, because editors like VSCode/WebStorm etc. have plugins to support typescript. Just if there are errors in the code, console doesn't inform about them, and still runs the compiled code. This is done to speed up performance. To prevent accidental errors on our main branches, we can should run tsc command on commit, which will do the type checking as part of compilation process.

More info: https://iamturns.com/typescript-babel/

  1. React typescript files should have *.tsx file extension, as it's the standard.

  2. Local JS files are recognized and type checked, so when .js functions/components are imported in .ts files they will throw errors most of the time. The best solution is to rewrite the javascript file to typescript, but sometimes we might not have the time to do that. Typescript can understand types in js files from type comments, so we can provide types or disable checking for those functions, mostly happens with components that have optional props e.g.:

//JS file
/** @type any */
export const IAmJavascriptCmp = ({ requiredProp, optionalProp }) => {
    return <>{requiredProp}</>;
}

//TS file
<IAmJavascriptCmp prop="test" />

IAmJavascriptCmp will be marked with an error, as optionalProp is assumed to be missing. @type override can prevent the errors (but should eventually be removed when files are rewritten in typescript)

  1. JS modules need type declarations. Most popular modules have types out of the box or can be installed with npm install --save -D @types/module-name. Our projects are setup to recognize typescript files, and do not require declarations for them. However, javascript files are not recognized, so they need declarations. For example, if we have proton-shared/lib/constants.js file and import it from .ts file: import { PERMISSIONS } from 'proton-shared/lib/constants'; it will say: Could not find a declaration file for module 'proton-shared/lib/constants'.

Best solution is to just rewrite the file to ts quickly, if the file is too large we can use any type for more complex declarations and just come back and fix those parts later. If that's not possible for some reason, we can declare a module in index.d.ts file:

declare module `proton-shared/lib/constants` {
	const value: any
	export = any
}

But that should be the last resort.

tsconfig.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@dhoko
Copy link
Contributor

dhoko commented Sep 23, 2019

we can should run tsc command on commit

We have lint-staged for this task, you can edit the config here https://github.com/ProtonMail/react-components/blob/master/package.json#L25

@vincaslt vincaslt force-pushed the feat/typescript branch 2 times, most recently from 746e13b to 46188b3 Compare October 1, 2019 07:33
@vincaslt vincaslt requested review from mmso, EpokK, econdepe and Yiin October 1, 2019 11:33
@EpokK EpokK merged commit 384f0b3 into master Oct 3, 2019
@EpokK EpokK deleted the feat/typescript branch October 3, 2019 14:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants