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

Interactivity API: Inability to use plain arrays in context #63651

Closed
2 tasks done
luisherranz opened this issue Jul 17, 2024 · 13 comments · Fixed by #62734
Closed
2 tasks done

Interactivity API: Inability to use plain arrays in context #63651

luisherranz opened this issue Jul 17, 2024 · 13 comments · Fixed by #62734
Labels
[Packages] Interactivity /packages/interactivity [Type] Bug An existing feature does not function as intended

Comments

@luisherranz
Copy link
Member

Description

Reported by @otakupahp in #60219 (comment)


Before 6.6 I had an array with other arrays:

array(
    array( 'id' => 1, 'item' => 'item 1 name' ),
    array( 'id' => 1, 'item' => 'item 2 name' ),
    array( 'id' => 1, 'item' => 'item 3 name' ),
);

That create this JSON:

[
    {"id":1, "item":"item 1 name"},
    {"id":2, "item":"item 2 name"},
    {"id":3, "item":"item 3 name"},
]

The Interactive API created a context object that contains the array without problems.

Now, it just renders a string instead of an array object

To fix the error, I have to modify my array into this:

array(
    'data' => array(
        array( 'id' => 1, 'item' => 'item 1 name' ),
        array( 'id' => 1, 'item' => 'item 2 name' ),
        array( 'id' => 1, 'item' => 'item 3 name' ),
    )
);

Which created this JSON:

{
    "data": [
        {"id":1, "item":"item 1 name"},
        {"id":2, "item":"item 2 name"},
        {"id":3, "item":"item 3 name"},
    ]
}

This is indeed parsed as an object.

Step-by-step reproduction instructions

Unknown yet, @otakupahp needs to provide more details.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@luisherranz luisherranz added [Type] Bug An existing feature does not function as intended [Packages] Interactivity /packages/interactivity labels Jul 17, 2024
@luisherranz
Copy link
Member Author

@otakupahp, could you please provide a detailed list of the steps we need to take to reproduce the problem, including the necessary code? Thanks.

@otakupahp
Copy link

Step-by-step reproduction instructions

Use the array provided as the context and try to access the context as an array

const context =getContext();
console.log( context[0].id );

You will get an error like this: Uncaught TypeError: Cannot access property on string '['

@luisherranz
Copy link
Member Author

Thanks. I was able to reproduce it using ^6.0.0, and indeed it was working before that (5.7.0 or lower).

You can reproduce it by changing the version here: https://stackblitz.com/edit/vitejs-vite-l8adg6?file=index.html,main.js,package.json

Two questions:

  • What should be the behavior? Should it be possible to a flat array to context? Or should it only be possible to assign an object?
  • @DAreRodz how does this work in your proxy refactoring? Can you take a look at it?

@sirreal
Copy link
Member

sirreal commented Jul 17, 2024

What should be the behavior? Should it be possible to a flat array to context? Or should it only be possible to assign an object?

If contexts are allowed to be arrays, why not any other type of value? Then it's up to the developer to do the right thing.

Contexts are intended to be accessed from directives, like data-wp-text="context.foo", in that sense it only makes sense to use collections.

My feeling is that it's best to only allow objects for state, store, and context values.

@luisherranz
Copy link
Member Author

My feeling is that it's best to only allow objects for state, store, and context values.

It indeed feels weird, as the only possible use case to reference a plain array in a directive would be to use it directly in data-wp-each:

<template data-wp-each="context">...</template>

In theory, this might also work, but it doesn't feel like something people should use:

<div data-wp-text="context.0.text"></div>

@otakupahp is there any use case for needing to use a flat array as the root of the context?

@otakupahp
Copy link

Yes, the plain array was the evolution of wp-each not working as expected.

I tried to use it, but it didn't load the content as needed.

@luisherranz
Copy link
Member Author

Are there any drawbacks to using a property to store the array in the context in your case?

{
  "data": [
    { "id": 1, "item": "item 1 name" },
    { "id": 2, "item": "item 2 name" },
    { "id": 3, "item": "item 3 name" }
  ]
}
<template data-wp-each="context.data">...</template>

@otakupahp
Copy link

My app is more complex than that. It uses wp.apiFetch to get data and update the DOM, so I use context inside the store.

That is why, using an array instead of an object wasn't a problem at all.

However, I had many problems adding DOM objects to the block because it didn't have access to the context, nor the directives (but that is a problem for another issue).

If you decide to go the "proper objects" only, that's fine, but please update the docs, so other devs don't face the same issue.

@DAreRodz
Copy link
Contributor

DAreRodz commented Jul 18, 2024

My feeling is that it's best to only allow objects for state, store, and context values.

I agree. In addition, allowing other instance types could undermine the context's extensible/inheritable nature. Let's consider this example:

<div data-wp-context='{ foo: "bar" }'>
  <span data-wp-text="context.foo"><!-- Will output: "bar" --></span>

  <div data-wp-context='["baz"]'>
    <span data-wp-text="context.0"><!-- Should output "baz" ? --></span>
    <span data-wp-text="context.foo"><!-- Should output "bar" ? --></span>
  </div>
</div>

The current implementation requires the context to be an object. The wp-context directive initializes> an empty object, proxies it to make it reactive, and then copies the content of the JSON specified in the wp-context value.

If you decide to go the "proper objects" only, that's fine, but please update the docs, so other devs don't face the same issue.

Yeah, I feel we need to expand the wp-context documentation a little. 😅

@luisherranz
Copy link
Member Author

If you decide to go the "proper objects" only, that's fine, but please update the docs, so other devs don't face the same issue.

Thanks @otakupahp for your understanding.

As you can see, although this was possible in the initial version, it seems like it should never have been there. Sometimes it's difficult to consider all possible use cases in the initial versions, and these use cases only become apparent when more people are using the functionalities.

So I'm sorry for the trouble and thanks for reporting the problem.

The current implementation requires the context to be an object.

@DAreRodz Can we add a test to ensure there is no regression? Thanks!

@otakupahp
Copy link

I found other issues that would be great if you can fix

@luisherranz
Copy link
Member Author

Sure, feel free to open an issue or a discussion reporting other problems, and we will take a look at them.

@otakupahp
Copy link

Looks like what I'm looking at is called Island Hydration... will check it more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Packages] Interactivity /packages/interactivity [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants