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

Update flow based on user feedback #3

Closed
wants to merge 1 commit into from
Closed

Conversation

jonathantneal
Copy link
Collaborator

@jonathantneal jonathantneal commented Feb 2, 2023

This PR attempts to improve the library, based on helpful feedback and suggestions from @louiss0 in #1 and #2.

  • Uses types as suggested where applicable.
  • Moves shared types to a types.d.ts.
  • Simplifies naming like getGenerator.
  • Omits version from internals, private packages.

Copy link
Contributor

@louiss0 louiss0 left a comment

Choose a reason for hiding this comment

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

First of I'm sorry for changing the entire codebase. I thought my version would change everything and make things better. I didn't want to do this but I was trying to build but after a while I realized I did not have to. During that time I changed the setup from the code that was downloaded. I don't want to go into the details but. There are problems with this PR

Inaccessible Index and Array.

The problem is that the index is not accessible

When people use the index they generally use it to decide when and how a component should appear.

React example of a component appearing based on a number being even or odd

<ul>
{
['Foo', "bar", "baz"].map((value, index)=> (
	index % 2 === 0 
	? <div>{value}</div> 
	: null
	)
)
}
</ul>

React example of a component class being added based on whether or not it is last or first

<ul>
{
['Foo', "bar", "baz"].map((value, index,array)=> (
	<div class:list={{
	'bg-gray-500': index === 0,
	'bg-violet-900': index === array.length - 1,
	
	}} >
		{value}
	</div> 
)
)
}
</ul>

The road to working with any iterable

My solution solves the above problems.

The HasForEachMethod interface makes sure that what is being passed in is something with a for each method.

interface HasForEachMethod {

forEach(callbackfn: (...args: Array<unknown>) => void, thisArg?: unknown): void;

}

The mapArgsFromIterableWithForEachMethodToArray is a small helper that grabs all of the arguments that come from each iteration of the iterable that is being passed in stores them inside of an array.

const mapArgsFromIterableWithForEachMethodToArray = (iterable: HasForEachMethod) => {

const arrayOfArgs = [];

iterable.forEach((...args) => {

arrayOfArgs.push(args);

});

return arrayOfArgs;

};

In the iterate function that I wrote uses mapArgsFromIterableWithForEachMethodToArray() to the pass the arguments from the iterable from the first into the callbackfn that is used as the second argument.

async function* iterate<T extends HasForEachMethod>(

iterable: T,

callbackfn: Parameters<T['forEach']>[0] | GeneratorFunction

) {

for await (const args of mapArgsFromIterableWithForEachMethodToArray(iterable)) {

if (isGenerator(callbackfn)) {

yield * callbackfn(...args)

}

else {

yield* wrapFunctionWithAsyncGenerator(callbackfn)(...args);

}

}

}

As you can see I was trying to make sure that the values that were passed to the callbackfn as the second argument would be the ones that come from the forLoop() of any data structure. This would ensure better typing and no terrible restrictions.

If you don't want to go with my solution to the iterate problem that is fine. But the index has to appear.

<For iterable={["Jack", "Dary"]}>
(_, index)=> index
</For>

No Case Typing's

The proper implementation of a Case Statement requires the use of either a case or default. The user should only be able to type one not the other.

interface FilledCase<T> {

of: T ;

default?: never;

}

interface DefaultCase {

of?: never;

default: true;

}

type Case<T> = FilledCase<T> | DefaultCase;

The type above is supposed to be used for <Case /> `

  • FilledCase of:T means that any value will be taken in and the value will be inferred but default: can't be typed.
  • DefaultCase means that of: can't be typed but default must be set to true

Putting FilledCase and DefaultCase together makes this type.

type Case<T> = FilledCase<T> | DefaultCase;

@louiss0
Copy link
Contributor

louiss0 commented Feb 4, 2023

I don't care about merging the two repo's anymore. Just to be clear. I do want some of the types that I have written to be incorporated though. Looking at the code that was inside of my repo will be a good reference to the problems that need to be solved.

P.S: I was not sure that you were happy when you wrote that last comment on the last PR but I was just wanted to be sure that you were available.

I made an awful mistake by basically rewriting the flow files from scratch and incorporating pnpm If you look at the code I have written I hope you see the trade off's between your solution and mine. You don't have to use my solution but the problems I'm trying to solve should be.

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