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

Fix #14 #15

Merged
merged 1 commit into from
May 9, 2023
Merged

Fix #14 #15

merged 1 commit into from
May 9, 2023

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented May 9, 2023

The PR fixes #14, and unblocks toeverything/AFFiNE#2257.

cc @himself65

@SukkaW SukkaW force-pushed the fix-call-expression branch from 5fd61cb to 8f3e19f Compare May 9, 2023 06:37
@lxsmnsyc
Copy link
Owner

lxsmnsyc commented May 9, 2023

I like it, but I have some new stuff being worked on in the next branch, might be good if you can rebase to that branch

@SukkaW
Copy link
Contributor Author

SukkaW commented May 9, 2023

I like it, but I have some new stuff being worked on in the next branch, might be good if you can rebase to that branch

Sure! Won't be a problem for me. I will mark my PR to draft then.

@SukkaW SukkaW marked this pull request as draft May 9, 2023 07:40
@SukkaW SukkaW force-pushed the fix-call-expression branch from 8f3e19f to de22afd Compare May 9, 2023 07:45
@SukkaW SukkaW changed the base branch from main to next May 9, 2023 07:45
@SukkaW SukkaW force-pushed the fix-call-expression branch from de22afd to dd5e7a9 Compare May 9, 2023 07:46
@SukkaW SukkaW marked this pull request as ready for review May 9, 2023 07:47
@SukkaW
Copy link
Contributor Author

SukkaW commented May 9, 2023

I like it, but I have some new stuff being worked on in the next branch, might be good if you can rebase to that branch

I have rebased the PR based on the next branch and updated the PR to target the next branch.

@SukkaW SukkaW force-pushed the fix-call-expression branch from dd5e7a9 to f12d131 Compare May 9, 2023 07:54
@lxsmnsyc
Copy link
Owner

lxsmnsyc commented May 9, 2023

Thanks! I can merge this right now however I still needed a way to so that it all works on all forms of expression, not just call expressions.

@lxsmnsyc lxsmnsyc merged commit b525224 into lxsmnsyc:next May 9, 2023
@SukkaW
Copy link
Contributor Author

SukkaW commented May 9, 2023

Thanks! I can merge this right now however I still needed a way to so that it all works on all forms of expression, not just call expressions.

Yeah, I just noticed that as well. And I have created a minimum reproduction:

import { useA, useB, useC, useD, useE } from 'something';
function Example(props) {
  return useA(useB(), [useC()], { d: useD() }, ...useE());
}

@lxsmnsyc
Copy link
Owner

lxsmnsyc commented May 9, 2023

Thanks! I can merge this right now however I still needed a way to so that it all works on all forms of expression, not just call expressions.

Yeah, I just noticed that as well. And I have created a minimum reproduction:

import { useA, useB, useC, useD, useE } from 'something';
function Example(props) {
  return useA(useB(), [useC()], { d: useD() }, ...useE());
}

ideally what we wanted here is to prevent generating/hoisting the dependencies but rather inlining it while partially memoizing the entire thing. In this case here it's not exactly memoizable because the rest doesn't have a constant part.

@SukkaW SukkaW mentioned this pull request May 9, 2023
@SukkaW
Copy link
Contributor Author

SukkaW commented May 10, 2023

ideally what we wanted here is to prevent generating/hoisting the dependencies but rather inlining it while partially memoizing the entire thing. In this case here it's not exactly memoizable because the rest doesn't have a constant part.

Yeah. But not only nested hook calls:

function Example(props) {
  let a = null;
  return { [useA()]: useB(), ...useC(), [\`testA\${useH()}testB\`]: useI() === useJ() }
}

forgetti currently can not handle this as well. I am working on #15 to see if I am able to fix this.

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.

Rendered fewer hooks than expected. This may be caused by an accidental early return statement.
2 participants