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

refactor: organize imports and types with import aliases #96

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

Conversation

brw
Copy link
Member

@brw brw commented Oct 28, 2024

No description provided.

@brw brw assigned brw and Tnixc Oct 28, 2024
@brw
Copy link
Member Author

brw commented Oct 28, 2024

Not sure if I really like it the way I have it now but just see what you think of my changes @Tnixc. I'm very much open to suggestions.

I'm not quite sure how to organize this stuff tbh. I had to move the search-parser and template-parser types to @shared, otherwise importing those in RequestAPI/ListenAPI would not work as that is also used by renderer. I kinda dislike having to move types to @shared as soon as they're used by the router/anything else though as I'd prefer to keep them near where they're first used/defined. Maybe there's a better solution that would allow for this.

I went through a lot of the tsconfig documentation, looked at many templates and examples, had many back-and-forths with ChatGPT and tried a bunch of different setups but I still don't feel like I know what the right approach here would be honestly lol.

Changing the .d.ts files to .ts did also uncover a few bugs/type errors as .d.ts files weren't actually being checked because skipLibCheck is enabled in the electron-toolkit tsconfig we extend from (we also just shouldn't use .d.ts files in general as they're unnecessary, see e.g. https://x.com/mattpocockuk/status/1820454283457765645).

Couple of notes:

  • I went back to @main and @renderer because I was having a very hard time getting this shit to work and I thought that maybe having the same alias for multiple paths was giving problems. Not sure if it actually was though. Could maybe go back to @/ for both but I'm not sure if I really like that anymore. Has the potential to be confusing.
  • NoticeType had a JSX Icon as a property but it was also imported in ListenAPI (I assume so that we can send toasts from the backend). This didn't work as you can't use/import JSX on the backend, so I removed the Icon property and re-added it on the front-end in a separate IconNoticeType type. This could use a refactor.
  • throttle.ts shouldn't really be in shared because setTimeout returns something different in Nodejs vs in the browser.

@brw brw changed the title chore: change imports to be absolute with aliases refactor: organize imports and types with import aliases Oct 28, 2024
@Tnixc
Copy link
Collaborator

Tnixc commented Oct 30, 2024

  • I suppose splitting it into @main and @renderer makes sense, it is less confusing
  • Is there no way to set an Icon when sending notices from the backend?
  • I agree with your point of avoid moving types to @shared and keeping them co-located with where they're used. We could import from @renderer in main and vice versa really, for something like Notice. I don't know what's good practice though

@brw
Copy link
Member Author

brw commented Oct 30, 2024

Is there no way to set an Icon when sending notices from the backend?

I don't think you can use JSX in the backend Electron process no and even if you could I don't think you'd want to do that. Better to just define a mapping of possible icons and specify the icon name when creating a notice.

We could import from @renderer in main and vice versa really, for something like Notice. I don't know what's good practice though

That's the thing, you can't. Not with our split tsconfig setup anyway. You can't import types from files not covered by the tsconfig. You can if you add a reference to the node tsconfig in the web tsconfig but then you essentially have web depend on node as a project, which means it needs to be built first to get the types from it. Probably not great either. I'm also not sure what the best practice here is other than just moving it to a shared package.

@Tnixc Tnixc marked this pull request as ready for review November 18, 2024 21:59
@Tnixc Tnixc requested a review from CaptSiro as a code owner November 18, 2024 21:59
@Tnixc Tnixc requested review from duduBTW and hrfarmer November 18, 2024 22:01
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