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

TypeScript - Stage 1 #531

Merged
merged 1 commit into from
Nov 16, 2020
Merged

TypeScript - Stage 1 #531

merged 1 commit into from
Nov 16, 2020

Conversation

LordSputnik
Copy link
Member

@LordSputnik LordSputnik commented Nov 8, 2020

Problem

Switching from Flow to TypeScript. Stage 1 on the TypeScript roadmap in https://tickets.metabrainz.org/browse/BB-557 (plus a dependency on typescript itself, from stage 3)

Solution

  • Install the Babel TypeScript preset, which allows babel to transpile TypeScript files to plain JavaScript.
  • Enable Webpack to resolve module names with .ts and .tsx extensions, and enable the babel-loader module for .ts and .tsx files
  • Add a basic, non-strict TypeScript configuration file, to faciliate conversion of existing flow-annotated files to TypeScript with a minimal diff
  • Add "typescript" and "@babel/preset-typescript" as development dependencies

Areas of Impact

Doesn't affect existing code or build process for JavaScript files.

@coveralls
Copy link

coveralls commented Nov 8, 2020

Coverage Status

Coverage remained the same at 60.997% when pulling 87ee5ee on LordSputnik:typescript into c3f90a5 on bookbrainz:master.

@LordSputnik
Copy link
Member Author

For some reason, Travis CI hasn't sent a "success" event back to GitHub, but the CI check has passed on Travis. I think this is ready to go (and once it's in, I can start putting together the PR to switch things over to TypeScript!)

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

All the suggestions look fine to me.

I'm getting an error when I try to compile the client-side javascript however. I checked that I don't get the error in the master branch.

Running npm run build-client, I get this error:

ERROR in /bookbrainz-site/node_modules/log/lib/private/logger-prototype/index.js
Module not found: Error: Can't resolve '../../../levels' in 'bookbrainz-site/node_modules/log/lib/private/logger-prototype'
 @ bookbrainz-site/node_modules/log/lib/private/logger-prototype/index.js 9:25-51
 @ bookbrainz-site/node_modules/log/index.js
 @ ./entity-editor/name-section/actions.js
 @ ./entity-editor/name-section/name-section-merge.js
 @ ./entity-editor/entity-merge.js
 @ multi ./entity-editor/entity-merge.js

Not sure what is causing the issue or how it is related to the introduction of typescript. I tried playing with the moduleResolution option in tsconfig, to no avail.

However I find it odd that we would use the node log package in a client page, and replacing it with a console.error statement the client-side JS compile successfully.
It was a mistake on my part that I introduced in a previous commit.

I propose for me to open a PR to remove the use of log in the client (it's a single occurrence, so a quick and easy one), so as to unblock this PR?

@LordSputnik
Copy link
Member Author

Thanks Monkey.

I did see that issue too locally, I did come up with a workaround but also thought it was odd that log was used client-side. If you make that change, I'm happy to rebase this.

@MonkeyDo
Copy link
Member

MonkeyDo commented Nov 12, 2020

Out of curiosity, what was your workaround?
I'm not even sure what the source of the problem is, nor why it relates to the introduction of TS.

Edit: OK, I removed the log from client code, it's ready to rebase.

@LordSputnik
Copy link
Member Author

Hmm, I thought I got around it by installing a package, but I think that may have been for typescript-eslint - we'll get to that one after another few PRs... I suspect for this I just commented out the logging! Will rebase this tonight.

@LordSputnik
Copy link
Member Author

Rebased on top of latest master branch, should compile now.

@LordSputnik LordSputnik reopened this Nov 14, 2020
@LordSputnik
Copy link
Member Author

Oops, misinterpreted the Android GitHub app interface there...

@MonkeyDo MonkeyDo merged commit 2c45b6a into metabrainz:master Nov 16, 2020
@MonkeyDo
Copy link
Member

Thanks for this first step !

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.

3 participants