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

Initial pass of adding Typescript #38

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

Samywamy10
Copy link
Contributor

@Samywamy10 Samywamy10 commented Aug 20, 2024

Adding Typescript support to the React project as per (my own 😆 ) issue #36 and also saw the comments here #14 (comment).

I wanted to add Typescript to the importable/public API so when consuming lvgljs its easy to see which components and APIs are available.

Example:

Screen.Recording.2024-08-20.at.7.02.49.PM.mov

Adding types

In this PR I've done an initial first pass at adding types across both the components as well as each of the styles. I've done the best I could to generate both the Prop Types as well as the Style Types based on available methods and also reading through the documentation. Something that might be good is to go through and compare to the Javascript generated by the .cpp files, but I'm not that familiar with C++.

I fully expect this to be a work in progress. I was trying to do the most useful types first but there are still lots of .js files across the codebase. Fortunately Typescript implementation can be somewhat progressive, in that .js and .ts files are interchangeable and I haven't turned on any build errors or linting rules for Typescript yet. We can view it as a useful enhancement/nice-to-have right now while we build out the full types.

As part of this, I've done my best to not modify any actual implementation - I've only added additional type annotations. There aren't any actual logic changes in the source code. There are quite a few errors/bugs that Typescript has identified so will do my best to address them in future PRs.

With that said, this already should be useful for consuming projects. E.g. looking at demo/hello_world/index.tsx we can see it both defines prop types for each component, as well as valid styles.

Build/import changes

I did make some slight changes to imports/builds to make Typescript work, and this moves to a more standard pattern of importing projects.

  • Remove the lvgljs-ui alias in build.js
  • Instead, import the /src/render/react/package.json project as lvgljs-ui in the root /package.json
  • This allows Typescript to properly import and understand types
  • Add react-reconciler and react types

Future PRs/TODOs

  • Convert rest of JS project to Typescript
  • Add Typescript errors/linting to build scripts & pipeline
  • Fix bugs/logic errors identified by Typescript
  • Refactor/remove some of the unused methods (eg a lot of the methods in each config.ts file aren't required and are duplicated)
  • Manually or automatically (open to ideas?) generate types of the Native components from C++

Thanks for reviewing, I've tried to keep this to a small scope (types for components + styles only) but I realise its still a big PR 😅 . This PR definitely needs to be squashed before merging. Hopefully this makes using lvgljs much easier for consumers!

@derekstavis
Copy link
Collaborator

I'm sooo excited about this!! I'll take a look sometime this week, but thank you so much!!

build('demo/*/*.jsx')
build('test/**/*.jsx')
build('demo/*/*.{jsx,tsx}')
build('test/*/*.{jsx,tsx}')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
build('test/*/*.{jsx,tsx}')
build('test/**/*.{jsx,tsx}')

@derekstavis
Copy link
Collaborator

derekstavis commented Aug 24, 2024

We should probably add a tsconfig file and as part of that include txiki.js typings as well such that the runtime typings are also available.

Something else I'd like to do is to create a shared types file where we can define common types for the events:

  • ChangeEvent (defined here)
  • ClickEvent (defined here)
  • SelectEvent (defined here).

Making those reusable will clean up a lot of typings. Also, beware that while stopPropagation is available, target and currentTarget aren't.

For the C++ code I think I have an idea on how to generate types from it using the macros. At this point I almost feel like we're reinventing JSI and React Native 🤣 but that's sort of the goal I guess. But it could also be as simple as hardcoding them in .d.ts files. That's a problem for the future though.

So far I like this. I've been testing locally and it works. My only nits are really the tsconfig (my editor does not know what to do without one), txiki runtime types and the events duplicated everywhere. If you can address these I think we should be able to merge them and evolve from there.

@Samywamy10
Copy link
Contributor Author

Have addressed your comments by adding the tsconfig.json file, the tiki runtime types and shared types for OnChangeEvent and OnClickEvent - I couldn't see where the Select event was supposed to be used (onPress?) so I didn't do that one but added the other two.

For something like onClick its implemented in the common API, and then again in some components. I don't think it needs to be implemented in each component if its in the base one but I didn't want to change any logic so can address in future.

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.

2 participants