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

Big project cleanup (use Typescript, move files around, update stuff, remove unused code, ...) #996

Merged
merged 52 commits into from
Feb 2, 2023

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Feb 2, 2023

Opencast Studio hasn't been worked on a ton over the past two years or so. Since then, we started two other projects internally (Tobira, Opencast Editor) and got more experience with React, Typescript and other things. This PR aims to bring the Studio codebase closer to our other projects to make future development easier and faster. This also includes just using more modern and idiomatic solutions.

The main things this PR does are:

  • Use Typescript: this makes it way easier to review PRs and trust future changes. I needed to fix quite a lot of errors that occurred after enabling typescript. Strict mode is enabled, but noImplicitAny is disabled as fixing all of those errors would have taken too much time (there are still roughly 60 errors).
  • Use tsx file extension: editors will now correctly pick syntax with JSX support
  • Files were moved around to improve the project structure.
  • Unused code was deleted
  • Quite a bit of code was also rewritten to be more idiomatic/modern/better
  • Eslint warnings now make the CI fail

See the commits for detailed information.

There are still quite a lot of things I'd like to do, but did not have the time for.These are collected here: #997

We use Typescript in almost all of our other projects and think it
offers great benefits. Thus we want to use it in Opencast Studio too.
Renaming the files to use .jsx or .tsx (so that editors correctly
highlight by default) was a plan anyway.

There are still tons of type errors in the code. Those are fixed in
the next commit(s).
The `ignoreTags` config is ignored when setting `ignoreEventsCondition`.
Further, `ignoreEventsCondition` should return a boolean, which leads
to a type error and I assume the returned `undefined` was always
interpreted as `false`. But lastly, we likely just want the default
behavior of the library. The current behavior already leads to a bug:
typing 'd' inside the form fields at the end downloads the video.
It's pretty old and everyone can (and should) just upgrade to 14.1.
The `window.AudioContext` is supported since 14.1.
The service worker was never installed/used. So let's remove it.
We were only using it for the test deployments. But our sentry server
seems to be broken. Probably already for a long time. No one checked
Studio Sentry for multiple years. And I dislike sending data without
the users consent.
We stopped using crowdin quite a while back.
We used constants before as then typos would be caught. But now we have
typescript which also finds typos in strings.
This is necessary for it to work in proper React 18 (i.e. `createRoot`).
We cannot yet use `createRoot` for other reasons.
This is... not great, but it brings us type safety and it does not
really have an obvious disadvantage, apart from a silly name. See the
comment in `theme.tsx` for more information.
This gets rid of the last strictNullCheck error
It still runs for the branches they push, so effectively the PRs will
have a CI status displayed.
@LukasKalbertodt
Copy link
Member Author

By the nature of this, this is awful to review. We knew from the beginning that this would not be reviewed. Before the next release it should be tested extensively to make sure this didn't introduce any bugs. But for now... merging.

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.

1 participant