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

Static Typing with TypeScript (WIP) #619

Closed

Conversation

bradenmacdonald
Copy link
Contributor

@bradenmacdonald bradenmacdonald commented Dec 15, 2023

Description:

This PR incrementally brings static typing and type checking (TypeScript) to frontend-platform. See OEP-67-8 TypeScript for some rationale.

To achieve this:

  • I added // @ts-check to the top of most .js files so that TypeScript will check them, and you'll see it in VS Code
  • The existing JSDoc comments are expanded and fixed to use TypeScript JSDoc syntax.
  • I fixed quite a few type errors that were now caught in the code.
  • I eliminated the dist folder and the existing build step. There's no need for it IMHO. If some MFE that's using this wants to do some kind of Babel transform on its bundle, that can still happen within the MFE build process, but there's no need to do it in advance.
  • But because TypeScript won't import JSDoc types from .js file in node_modules we do still need a step to collect the JSDoc types and write them out to the typings folder. This is the new, simplified build step.

This should be 100% compatible with existing MFEs that are using this, though they might start seeing type errors if they are using typescript and have some actual errors.

This is also compatible with projects that want to use "moduleResolution": "Node16", "module": "Node16" in their tsconfig.json, i.e. use the exports key of package.json instead of the legacy approach. You can test this by putting "type": "module" in your MFE's package.json and "moduleResolution": "Node16", "module": "Node16" into tsconfig.json compilerOptions. However, most MFEs seem to be using the legacy CJS approach.

TODO:

  • Get the GitHub build green again (jest tests are already passing)
  • Add testing instructions, for both modules and non-modules consumers
  • verify backwards compatibility
  • figure out if JSDoc changes have impacted the docs
  • expand the typing of ConfigDocument to reflect custom keys unique to each MFE ?
  • Change eslint import resolver to typescript

Examples from an MFE

These screenshots are from the Course Authoring MFE. You can see that before this PR, VS Code is not aware of the type information of any of the frontend-platform functionality. (How do you live like this? :p )

Before After
Screenshot 2023-12-15 at 1 30 00 PM
intl and config are both any
Screenshot 2023-12-15 at 1 29 14 PM
Notice the types of both intl and config are now correct.
Screenshot 2023-12-15 at 1 33 13 PM Screenshot 2023-12-15 at 1 32 41 PM

Merge checklist:

  • Consider running your code modifications in the included example app within frontend-platform. This can be done by running npm start and opening http://localhost:8080.
  • Consider testing your code modifications in another local micro-frontend using local aliases configured via the module.config.js file in frontend-build.
  • Verify your commit title/body conforms to the conventional commits format (e.g., fix, feat) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.

Post merge:

  • After the build finishes for the merged commit, verify the new release has been pushed to NPM.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 15, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Dec 15, 2023

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

@bradenmacdonald bradenmacdonald changed the title Static Typing with TypeScript Static Typing with TypeScript (WIP) Dec 15, 2023
@mphilbrick211
Copy link

Hi @bradenmacdonald! Just checking in on this to see if you're still working on it?

@bradenmacdonald bradenmacdonald marked this pull request as draft March 21, 2024 21:13
@bradenmacdonald
Copy link
Contributor Author

@mphilbrick211 I ran into some issues and kind of lost the momentum here. I'm going to mark as a draft for now and not sure when I'll be able to revisit it. I would still like to move it forward at some point though.

@bradenmacdonald
Copy link
Contributor Author

I'm going to close this, and open a few smaller PRs with some of the same changes.

@openedx-webhooks
Copy link

@bradenmacdonald Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

1 similar comment
@openedx-webhooks
Copy link

@bradenmacdonald Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants