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

Organize utils.ts into folder, update imports #1332

Merged
merged 34 commits into from
Jun 20, 2023

Conversation

alectrocute
Copy link
Contributor

@alectrocute alectrocute commented Jun 16, 2023

Hi Lemdevs!

This is my first milestone of refactoring the utils.ts file into a folder with logical structure:

utils/
├─ browser/ – browser/`navigator`-related
├─ helpers/ – simple js/ts helper functions
├─ roles/ – utils related to checking user roles/permissions
├─ types/ – y'know... types and interfaces, etc.

Next PR will expand upon this, adding more folders as necessary. The finish line isn't yet in sight, but I'm confident I can finish the entire refactor/tidying project. utils.ts can be a thing of the past soon. 😄

Thanks so much!

@alectrocute
Copy link
Contributor Author

Wow, I totally ruined this branch. Somehow I pulled in a bunch of arbitrary changes, need to sort this out.

@alectrocute
Copy link
Contributor Author

alectrocute commented Jun 16, 2023

Resolved it. Please squash on merge when/if possible.

import { UserService } from "../../services";
import { amAdmin } from "./am-admin";

export function canCreateCommunity(
Copy link
Member

@SleeplessOne1917 SleeplessOne1917 Jun 17, 2023

Choose a reason for hiding this comment

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

If we're using a single function, a default export may be better. Same for other similar files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure I understand. 😅

Copy link
Member

Choose a reason for hiding this comment

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

I meant default export lol. No idea where subreddit came from.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! There's still a check that's failing lint.

src/shared/components/app/navbar.tsx Outdated Show resolved Hide resolved
@alectrocute
Copy link
Contributor Author

Not sure why CI is failing, but I'd love to get this merged and get started on the second pass. 🙌

Copy link
Member

@SleeplessOne1917 SleeplessOne1917 left a comment

Choose a reason for hiding this comment

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

The only change I would make (which I messed up saying earlier) would be to use default exports for single-function modules. E.g., for can-share.ts,

export default function canShare() {
  return isBrowser() && !!navigator.canShare;
}

instead of

export function canShare() {
  return isBrowser() && !!navigator.canShare;
}

@alectrocute
Copy link
Contributor Author

alectrocute commented Jun 20, 2023

@SleeplessOne1917 I'm planning on making those changes ASAP. Totally slipped my mind!

@alectrocute
Copy link
Contributor Author

Just need to fix the imports later tonight and we'll be good to go! Will comment and tag reviewers when it's ready. Thanks for your patience.

@jsit
Copy link
Contributor

jsit commented Jun 20, 2023

I've found that a common pattern is to set up an index.ts inside of each utils subdirectory, and then you can do things like:

import {
  amAdmin,
  amCommunityCreator,
  amMod,
  canAdmin,
  canMod,
  isAdmin,
  isBanned,
  isMod,
} from "@utils/roles";

Especially if we also set up paths in tsconfig.json:

https://www.typescriptlang.org/tsconfig#paths

Just a convention though, I also understand the desire to be as explicit as possible and not make imports at all mysterious.


Also is this PR re-writing any utils, or just organizing them? If it's just organizing them, I think it would be good to make that super clear in the PR description/title, so that it can be more easily reviewed without worrying if it's breaking anything.

@SleeplessOne1917
Copy link
Member

Also is this PR re-writing any utils, or just organizing them? If it's just organizing them, I think it would be good to make that super clear in the PR description/title, so that it can be more easily reviewed without worrying if it's breaking anything.

I second this.

@alectrocute
Copy link
Contributor Author

Also is this PR re-writing any utils, or just organizing them?

Just organizing for now. I'll update the PR title!

I've found that a common pattern is to set up an index.ts inside of each utils subdirectory, and then you can do things like...

Ooh, I like this.

@alectrocute alectrocute changed the title Refactor utils.ts into folder, update imports Organize utils.ts into folder, update imports Jun 20, 2023
@alectrocute
Copy link
Contributor Author

@jsit @SleeplessOne1917 Would you mind taking a look at my changes? For some reason, I'm getting Module not found errors for the paths I setup in tsconfig.json. What am I doing wrong? Aside from the errors, the paths seem to resolve just fine in my IDE.

@alectrocute
Copy link
Contributor Author

@jsit @SleeplessOne1917 @dessalines Upon adding TSConfigPathsWebpackPlugin, we're good to go!

Let me know if you'd like any other changes. This is ready for final review.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@alectrocute
Copy link
Contributor Author

Looks good, thanks!

You're welcome! Immediately after we merge this, I'll get started on the second pass which should completely eliminate our utils.ts file.

@jsit
Copy link
Contributor

jsit commented Jun 20, 2023

Thanks @alectrocute! Is adding this extra dependency necessary? I wonder if it would be cleaner just to manually do resolve in the webpack config.

@alectrocute
Copy link
Contributor Author

alectrocute commented Jun 20, 2023

Thanks @alectrocute! Is adding this extra dependency necessary? I wonder if it would be cleaner just to manually do resolve in the webpack config.

Any idea how to do this without the dependency?

It would definitely be cleaner to do it manually, but I'm unsure how at the moment.

@SleeplessOne1917 SleeplessOne1917 merged commit 94bc909 into LemmyNet:main Jun 20, 2023
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.

4 participants