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

Rules for property accesses #4

Open
yuchi opened this issue Nov 29, 2018 · 9 comments
Open

Rules for property accesses #4

yuchi opened this issue Nov 29, 2018 · 9 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@yuchi
Copy link
Owner

yuchi commented Nov 29, 2018

Because accessing a property in a callback could reference different values that the ones are available at function definition, we currently do not add property accesses to the inputs array. For example:

function MyComponent({ config }) {
  return useAutoMemo(config.value * 2);
}

Gives this output:

function MyComponent({ config }) {
  return useMemo(() => config.value * 2, [config]);
}

This is a big issue when dealing with refs, which are a { current } object. Since they are mutable, this makes the problem non-trivial.

Note: the props reference is a special case of this. Since is easier to identify, we should at least treat that some love.

@yuchi yuchi added enhancement New feature or request help wanted Extra attention is needed labels Nov 29, 2018
@adrianhelvik
Copy link

Idea: When accessing .current on a variable, it must be explicitly labelled or inferred to either be a ref or a non-ref. Assignment to useRef would automatically label it as a ref, as would wrapping it in the new macros markRef and markNonRef.

Triggers warning:

function App({ myRef /* is ref: unknown */ }) {
  useAutoEffect(() => {
    myRef.current
  })
  return null
}

Does not trigger warning:

function App({ myRef }) {
  markRef(myRef) // is ref: true
  useAutoEffect(() => {
    myRef.current
  })
  return null
}
function App({ myRef }) {
  markNonRef(myRef) // is ref: false
  useAutoEffect(() => {
    myRef.current
  })
  return null
}
function App() {
  const myRef = useRef() // is ref: true
  useAutoEffect(() => {
    myRef.current
  })
  return null
}
function App({ myRef /* is ref: unknown */ }) {
  const { current } = myRef
  useAutoEffect(() => {
    current
  })
  return null
}

@adrianhelvik
Copy link

If the default behavior is to add the object to dependencies and show a warning, it would probably not break a lot of code.

@adrianhelvik
Copy link

The algorithm

  • If .current is accessed in the scope or child scope of an auto hook
    • Find its highest parent (foo.bar.current -> foo)
    • Determine status of highest parent.
      • If maybeRef, throw error.
      • If isRef, add top parent to dependencies.
      • If isNonRef, add property access to dependencies.

Determine status

  • If defined in hook: nonRef
  • Go to component function scope
    • If defined by React.useRef: isRef
    • If defined by React.use*: isNonRef
    • If called with markRef in component function scope: isRef
    • If called with markNonRef in component function scope: isNonRef
  • Otherwise: maybeRef

Error message

Could not determine if ${expression} was ref or not.

Include `markRef(${topParent})` or `markNonRef(${topParent})`
in the function body to specify whether `${topParent}.*.current`
or is a React ref.

markRef/markNonRef

  • For each argument:
    • Assert that it's an identifier and that it is in scope
  • Remove call expression

@neoncube2
Copy link

Is it possible to have useAutoMemo() generate a variable with a munged name and then use that variable when memoizing?

e.g.

function MyComponent({ config }) {
  const __asdfanasdfjl__ = config.value;
  return useMemo(() => __asdfanasdfjl__ * 2, [__asdfanasdfjl__]);
}

@yuchi
Copy link
Owner Author

yuchi commented Jan 26, 2021

@neoncube2 yeah that is the plan, the problem is that accessing a property is not pure/idempotent (think property getters) and some ordering may need to be respected.

@adrianhelvik your approach is actually very clean! I’ll look into it! (Sorry for the huge delay!)

@adrianhelvik
Copy link

No worries. That's great news! 🙂

@neoncube2
Copy link

neoncube2 commented Feb 10, 2021

Thanks, @yuchi. I wonder if the ordering really needs to be respected, though? My understanding is that render() is already supposed to be pure, so it seems like we should be able to access properties as many times as we like without side effects 🙂

@yuchi
Copy link
Owner Author

yuchi commented Feb 16, 2021

@neoncube2 Yes, render functions are pure, but callbacks and effect callbacks can (and usually indeed are) impure.

@adrianhelvik
Copy link

adrianhelvik commented Feb 16, 2021

Some important things to consider is conditionals, this-context and impure getters. This contrived example:

const [config, configure] = useState()

const ref = useAutoCallback(element => {
  configure({
    actions: {
      click() { element.click() }
    }
  })
})

useAutoEffect(() => {
  if (!element) return
  config.actions.click()
})

return <button ref={ref}>Click me!</button>
(Train of thought)

Would break if transpiled into:

var _click = config.actions.click

useEffect(() => {
  if (!element) return
  _click()
}, [_click])

An alternative would be to transpile into:

var _click = config?.actions?.click

useEffect(() => {
  if (!element) return
  _click()
}, [_click])

But that would change the this context, if the ref looked like this for example:

const ref = useAutoCallback(element => {
  configure({
    element,
    click() { this.element.click() }
  })
})

So the original input...

Should leave us with something like:

// ...

var _click = config?.actions?.click
useEffect(() => {
  if (!element) return
  _click.call(config.actions)
}, [element, _click])

// ...

Edit: Then again, I don't think supporting impure getters should be the highest priority as it complicates the implementation for minimal gain. Without support for impure getters, there will be no need to handle this-context.

// ...

useEffect(() => {
  if (!element) return
  config.actions.click()
}, [element, config?.actions?.click])

// ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants