nukeop / nuclear

Streaming music player that finds free music for you
https://nuclear.js.org/
GNU Affero General Public License v3.0
11.72k stars 1.02k forks source link

Updated TrackResults index.js to Typescript #1598

Closed BMischley closed 2 months ago

nuki-chan[bot] commented 2 months ago

Lines 2-9

Nuki sees what you did there, converting this file to TypeScript—very trendy! 😏 But oopsie-daisy, looks like you forgot to remove lodash when you've got the power of TypeScript at your fingertips. TypeScript types are like magical girl transformations; they make everything better without the need for lodash!

-import _ from 'lodash';

😸 Nuki suggests rewriting the logic to use native JS methods and TypeScript features!


Lines 11-24

Kawaii types you've got there! 😍 But your Artist and Track can be more than optional properties floating around in space. They want to be required sometimes!

💡 It may look something like this:

type Artist = {
  name: string;
}

type Track = {
  name: string;
  image?: string;
  thumbnail?: string;
  artist: Artist | string;
}

Don't forget to adjust the logic accordingly to handle non-optional properties!


Lines 28-40

A wild TypeScript React.FC appears! ✨ And it’s followed by a band of Props. Pretty cool, but let’s be real: defaultProps don’t really gel with functional components and TypeScript. Perhaps consider using default function parameters or logical OR (||) for your default values, okay? To preserve the cuteness, of course.


Lines 23-25

And look at you, introducing TracksResultsProps. A tip of the hat to you, senpai, for defining props like a pro! But hey, why shy away from making tracks required? Give them the spotlight they deserve! ⭐

  tracks: Track[];

It'll help you remove that unnecessary tracks || [] later on, and simplify the whole operation. Don't worry, Nuki will guide you through it. 😉


Lines 28-40

Coming down to TrackRowProps, good job on making them so detailed! But again, you're defining props with optional fields, which is oh so mysterious, like a ninja in the night! Are these fields truly optional for TrackRow, or are you just being cautious? 🤔 Trust in your code's destiny and make those fields shine! ✨


Line 34

Uh-oh, we're returning a string when we should be returning JSX. Good catch on wrapping the t('empty') in a fragment (<></>)! It's like finding that hidden track on your favorite anime soundtrack. 😲 But do you think you could make it even more on-brand by returning a Segment with some sassy empty state text? Give it some personality!


Lines 42-55

The logic in this mapping reminds me of a puzzle that even L would have trouble solving! 🧐 So many lodash functions, so little time... Let's purify this with the power of TypeScript and ES6+, shall we?

You can get rid of all those _.hasIn checks because TypeScript's static type checking has got your back! And when you're cloning deep, remember: "Flat is better than nested." – The Zen of Python, which applies here too! Just keep things simple!

💡 Nuki thinks this can be vastly improved:

{collection.slice(0, limit).map((track, index) => {
  if (track.name && (track.image || track.thumbnail) && track.artist) {
    const displayArtist = typeof track.artist === 'string';
    const newTrack: Track = {
      ...track,
      artist: displayArtist ? { name: track.artist } : track.artist,
    };
    const trackRowProps: TrackRowProps = {
      track: newTrack,
      displayArtist: true,
      displayCover: true,
    };
    ...
    // Rest of your code

Nuki just expects you to write some fabulous tests for all this, right? GOOD! Because every new refactor needs to be test-driven, like not skipping the opening theme of your favorite show! 🎶

Remember, following Nuki's advice isn't mandatory for getting the PR merged, and she's still perfecting her mastery of the coding arts. If your heart tells you otherwise, follow it. ❤️