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

added an explicit typecast for Ember Array #23609

Closed
wants to merge 1 commit into from
Closed

Conversation

mrisher
Copy link

@mrisher mrisher commented Jan 13, 2023

This may be not be the best change, but I'm running Ember CLI 4.9.2 and needed to make this change to get past the error on pushObject() and enumeration. Is there a better way to fix this? Something got deprecated/changed in 4.x that is making your example (and most of what I find on StackOverflow) not work.

Description

Motivation

Additional details

Related issues and pull requests

This may be not be the best change, but I'm running Ember CLI 4.9.2 and needed to make this change to get past the error on `pushObject()` and enumeration. Is there a better way to fix this? Something got deprecated/changed in 4.x that is making your example (and most of what I find on StackOverflow) not work.
@mrisher mrisher requested a review from a team as a code owner January 13, 2023 11:44
@mrisher mrisher requested review from Elchi3 and removed request for a team January 13, 2023 11:44
@github-actions github-actions bot added the Content:Learn Learning area docs label Jan 13, 2023
@mrisher
Copy link
Author

mrisher commented Jan 13, 2023

This seems like the wrong solution, given the deprecation of A() I just learned about on the Discord (https://rfcs.emberjs.com/id/0848-deprecate-array-prototype-extensions/). So maybe we would need to rewrite the functions using todos instead of merging this change...

@mrisher
Copy link
Author

mrisher commented Jan 13, 2023

Okay, seems like also adding this.todos = this.todos; // self-assignment to trigger Tracked and replacing pushObject() with push would do the trick and be compliant. You'll also have to replace the filter with return this.todos.filter((todo) => todo.isCompleted == false);

@@ -214,6 +214,8 @@ This is because Ember extends JavaScript's Array prototype by default, giving us
There are dozens of these methods, including `pushObjects()`, `insertAt()`, or `popObject()`, which can be used with any type (not just Objects).
Ember's [ArrayProxy](https://api.emberjs.com/ember/4.2/classes/ArrayProxy) also gives us more handy methods, like `isAny()`, `findBy()`, and `filterBy()` to make life easier.

**Note:** If you receive a console error around the `pushObject()` function (e.g. `this.todos.pushObject is not a function`) you may need to explicitly typecast your JavaScript Array into an Ember Array using the `A()` method. Add `import { A } from '@ember/array';` at the top of the `todo-data.js` file, and replace `@tracked todos = []` with `@tracked todos = A()`.

Choose a reason for hiding this comment

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

I think, instead of adding this disclaimer, we should update to using TrackedArray, which has regular push <3

@mrisher
Copy link
Author

mrisher commented Jan 13, 2023 via email

@NullVoxPopuli
Copy link

NullVoxPopuli commented Jan 13, 2023

Welcome! No worries! It is newish, and does need to be installed, there is an RFC that proposed adding it by default, so i suppose it'll show up by default fairly soon™

Your contributions mean a lot! <3

@Elchi3
Copy link
Member

Elchi3 commented Feb 20, 2023

@mrisher Thanks for your PR! Do you want to get back to it and make the changes @NullVoxPopuli proposed?

@Elchi3
Copy link
Member

Elchi3 commented Mar 14, 2023

@mrisher friendly ping
(This PR might get closed due to inactivity soon)

@mrisher
Copy link
Author

mrisher commented Mar 15, 2023

So Sorry, I overlooked this note before and forgot about this. I will check whether I can implement TrackedArray myself in the next few days, if not I'll call for help. Thank you!

@mrisher
Copy link
Author

mrisher commented Mar 15, 2023

Hi: I tried to update to use TrackedArray tonight and am hitting bugs; I may not be familiar enough with the Ember internals to debug -- basically, I updated the variable to todos = new TrackedArray([]); and removed the self-assignment and everything stopped working with no visible errors.
Happy to try if someone can help me out, otherwise I think we should retire this PR.

@NullVoxPopuli
Copy link

can you provide the code you had?, the whole file, ideally? <3

@mrisher
Copy link
Author

mrisher commented Mar 16, 2023

Sure thing. This is the file adapted from your example code, where todos is defined on line 53 and then self-assigned on line 87 to force the update. Last night I tried changing line 53 to todos = new TrackedArray([]); and commenting out the self-assignment (after installing the tracked-built-ins package) but then my array stopped updating.

Maybe I need to declare the object type for the TrackedArray? And IIUC the TrackedArray supports push() and filter() so I assumed I didn't need to rewrite those, but maybe (I didn't have time to debug further).

Thanks for the help!

@NullVoxPopuli
Copy link

NullVoxPopuli commented Mar 16, 2023

@mrisher in that repo you provided, does it work without firebase? I don't have an account

but then my array stopped updating.

what you did should have worked 🤔
I see that here: https://github.com/mrisher/junkdrawer/blob/secret-env/app/services/todo-data.js#L92
the tracked todos is set back to a normal array instead of a TrackedArray -- when using tracked array, you'll want to opt for mutation over replacing the reference to the array

@mrisher
Copy link
Author

mrisher commented Mar 16, 2023

Ah, I see the problem, it's the assignment when it reads the json blob back from firebase here. Is there an idiomatic way to do a deep copy instead of assignment at this line? Or should I rewrite that whole deserializeJson() method to take a pointer to the TrackedArray? Thank you for your patience. I'm not aware of an easy way to stub out the firebase code for you...

@Elchi3
Copy link
Member

Elchi3 commented Apr 26, 2023

@mrisher @NullVoxPopuli sorry, seems like this is stuck (again). I'm closing. Feel free to come back to it, though.

@Elchi3 Elchi3 closed this Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Learn Learning area docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants