cibernox / ember-power-select

The extensible select component built for ember.
http://www.ember-power-select.com
Other
539 stars 377 forks source link

Possible bugs with EPS v4.0.0-beta.2 #1296

Closed ijlee2 closed 4 years ago

ijlee2 commented 4 years ago

Hello. I'm following an Ember tutorial and rewriting the app in Octane (Ember v3.13.1) as I go. I met two problems when it was time to introduce EPS and I wanted to try out the latest 4.0.0-beta.2.

If you have questions or need clarifications, feel free to let me know. You can find my latest code here: https://github.com/ijlee2/Tutorials/tree/master/ember-embercasts-library-ui. Thanks!

1. Compile error: assign is not a helper

Once I added a <PowerSelect> in a template, I encountered the error above. On Ember Discord, madnificent kindly pointed out that one needed to install ember-assign-helper.

This addon is currently listed under devDependencies in package.json. Does it need to be under dependencies instead?

2. Unable to click on an option if I provide an array of Ember Data records

When @options is an array of Ember Data records, the selectAuthor below doesn't get called when I click on an option. However, if I use the arrow keys and Enter to select, the action is called.

I think the problem only occurs with an array of ED records. If this.model is an array of strings or an array of POJOs, I am able to call the action with mouse or Enter key.

Similarly, when I pass to @search an action that returns an array of ED records, I am unable to click on an option.

// Route template

...
<PowerSelect
    @options={{this.model}}
    @selected={{this.selectedAuthor}}
    @onChange={{this.selectAuthor}}
    as |author|
>
    {{author.lastName}}, {{author.firstName}}
</PowerSelect>
...
// Route handler

import Route from '@ember/routing/route';

export default class BooksCreateRoute extends Route {
    model() {
        return this.store.findAll('author');
    }
}
// Controller

import Controller from '@ember/controller';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';

export default class BooksCreateController extends Controller {
    @tracked selectedAuthor;

    @action
    selectAuthor(author) {
        this.selectedAuthor = author;
    }
}

Temporary workaround

In case there are others who want to work with EPS and ED, I'll post my temporary solution here:

// Route template

...
<PowerSelect
    @selected={{this.selectedAuthor}}
    @searchEnabled={{true}}
    @search={{this.searchAuthor}}
    @onChange={{this.selectAuthor}}
    as |author|
>
    {{author.lastName}}, {{author.firstName}}
</PowerSelect>
...
// Route handler

import Route from '@ember/routing/route';

export default class BooksCreateRoute extends Route {
    model() {
        return {
            title: '',
            isbn: '',
            publicationDate: null,
            author: null
        };
    }
}
// Controller

import Controller from '@ember/controller';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';

export default class BooksCreateController extends Controller {
    @tracked selectedAuthor;

    @action
    async searchAuthor(query) {
        const authors = await this.store.query('author', {
            filter: { query }
        });

        // Ember Power Select v4.0.0-beta.2 does not handle an array of
        // Ember Data models as options. To get around this issue, we can
        // return an array of POJOs and keep a record of searched authors.
        this.possibleAuthors = authors;

        return authors.map(author => ({
            id: author.id,
            firstName: author.firstName,
            lastName: author.lastName
        }));
    }

    @action
    selectAuthor(author) {
        this.model.author = this.possibleAuthors.find(possibleAuthor => {
            return possibleAuthor.id === author.id;
        });

        this.selectedAuthor = author;
    }

    @action
    saveBook(event) {
        event.preventDefault();

        const book = this.store.createRecord('book', this.model);

        book
            .save()
            .then(() => {
                this.transitionToRoute('books');
            });
    }
}
MikiDi commented 4 years ago

I can confirm both. For nr. 2 I believe it's here where things go wrong. Looks like direct use of objectAt was dropped a whole while ago (#bf87418148c2dc75aa9a0c74c4c164284c66bb11, PR #534 ) ( @madnificent You're mentioned up here ^ :+1: )

lucacorti commented 4 years ago

I have a slight variation of nr. 2. In my case when options is an array of ED records, selecting works about right. Also clearing seems to work (the action is called with null) but the selection is not cleared in the UI. Works fine in 3.x.

cibernox commented 4 years ago

@MikiDi / @lucacorti @ijlee2 Since I don't do thanksgiving, tomorrow I should have time to track this down. If any of you can come up with a sample repo where this can be easily reproduced that would help a lot.

ijlee2 commented 4 years ago

Thanks, @cibernox. I'll see if I can create a sample repo. Others are welcome to as well!

cibernox commented 4 years ago

@ijlee2 fwiw, I think I was just able to reproduce it with the snippets you pased above and using ember-cli-mirage to simulate a server

ijlee2 commented 4 years ago

Ah, cool. Yeah, I was going to extract the code out and introduce Mirage.

cibernox commented 4 years ago

@MikiDi / @lucacorti @ijlee2 try 4.0.0-beta.4, I think this should be fixed now

ijlee2 commented 4 years ago

Will do. Thank you!

lucacorti commented 4 years ago

@cibernox I'm still experiencing the issue described above with beta 4.

cibernox commented 4 years ago

@lucacorti I reproduced the issue and fixed it using the steps given by @ijlee2

Can you provide a reproduction step? I that helps, you can create a PR in this repo using the route /playground to put the reproduction in there.

ijlee2 commented 4 years ago

@cibernox I think I may be also still running into a problem with clicking on an option with beta.4. I've extracted my repo and added Mirage routes to help you with testing: https://github.com/ijlee2/embercasts-library-ui .

I wrote the following code to get the click working: https://github.com/ijlee2/embercasts-library-ui/blob/master/app/components/book-form/component.js#L21-L28

// This (using toArray) works, but doesn't quite seem right.
@action
async searchAuthor(query) {
    const authors = await this.store.query('author', {
        filter: { query }
    });

    return authors.toArray();
}

If I use this code instead, both options below fail for me.

// I feel like this is what EPS intended me to do.
// (I'm not sure, however, as I've been using an array of POJOs in the past.)
@action
searchAuthor(query) {
    return this.store.query('author', {
        filter: { query }
    });
}

Option 1

After npm install, run ember s and visit http://localhost:4200/books/1/edit. You can search for an author by typing a letter. The user should be able to click on an option afterwards.

Screen Shot 2019-11-30 at 1 13 38 PM

Option 2

You can run ember t --server. I wrote a rendering test that checks whether the user can choose an option: https://github.com/ijlee2/embercasts-library-ui/blob/master/tests/integration/components/book-form/component-test.js#L78-L79

cibernox commented 4 years ago

@ijlee2 ok, I found it and fixed it. I'll have it released soon

cibernox commented 4 years ago

@ijlee2 try updating once again to 4.0.0-beta.5

ijlee2 commented 4 years ago

@cibernox beta.5 worked for me, thanks!

lucacorti commented 4 years ago

@cibernox still seeing this in b5. The selected object is a Proxy. Not sure if this makes any sense to you. I'll try to setup a reproducible example as you suggested.

lucacorti commented 4 years ago

I can work around this with @ijlee2 workaround (.map()ing e-d records to plain objects).

cibernox commented 4 years ago

@lucacorti I might make sense. I fixed it for when the options are a proxy, also for when the result of the search action is a proxy, the selected property being a proxy is last remaining case

cibernox commented 4 years ago

@lucacorti ok, I believe that beta.6 should have your issue fixed now. It turns out the test suite around ember-data wasn't very exhaustive, now all those scenarios should be covered to avoid future regressions.

lucacorti commented 4 years ago

@cibernox The selected choice label is still not updating correctly for me in b6 when clearing the selection.