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

Updates needed to complete adoption to typesense github org #1

Closed
8 tasks done
jasonbosco opened this issue Jul 2, 2021 · 29 comments
Closed
8 tasks done

Updates needed to complete adoption to typesense github org #1

jasonbosco opened this issue Jul 2, 2021 · 29 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@jasonbosco
Copy link
Member

jasonbosco commented Jul 2, 2021

This repo was adopted from @AbdullahFaqeir's work here: https://github.com/devloopsnet/laravel-scout-typesense-engine.

The following tasks need to be done to complete the adoption:

  • Add LICENSE file (current license MIT)
  • Add credits to Abdullah Al-Faqeir and his company DevLoops for building the original version, in README
  • Change instances of Devloops in class names to Typesense
  • TypesenseServiceProvider.php has references to master_node and read_replica_nodes, which are old configuration parameters no longer supported in the latest version of Typesense Server. Needs to be switched to nodes and nearest_node
  • Publish new version of package under typesense namespace - @jasonbosco
  • Test that new published version works fine
  • Mark devloopsnet/laravel-typesense as abandoned in Packagist and list this package as its replacement
  • Update README.md in devloopsnet/laravel-scout-typesense-engine repo to point people to this repo: Adopting this repo into the typesense github org devloopsnet/laravel-scout-typesense-engine#17
@jasonbosco jasonbosco added the help wanted Extra attention is needed label Jul 2, 2021
@hi019
Copy link
Collaborator

hi019 commented Jul 6, 2021

We should also mark devloopsnet/laravel-typesense as abandoned in Packagist and list this as its replacement, assuming that is the case

@jasonbosco
Copy link
Member Author

Ah, didn't know that was a thing. Thanks @hi019. I've added it to the list.

@jasonbosco
Copy link
Member Author

Also, do you want to take a stab at making these updates? :)

@hi019
Copy link
Collaborator

hi019 commented Jul 6, 2021

Sure, I'll do that. I did a similar thing for https://github.com/hi019/laravel-scout-typesense :)

@hi019
Copy link
Collaborator

hi019 commented Jul 6, 2021

I'll also implement my other changes from my fork here when I have time

@jasonbosco
Copy link
Member Author

Awesome! Thank you. I'll assign this issue to you, so others know that you're working on it.

@jasonbosco
Copy link
Member Author

@hi019 On a side note, I'd love to invite you to join our Slack community in case we need to chat in real-time.

@manavo
Copy link
Contributor

manavo commented Aug 2, 2021

Guess the issue description at the top needs updating now that the PR is merged? Not sure what’s left now, or is the migration complete?

@jasonbosco
Copy link
Member Author

@hi019 Mind updating the checklist in this PR's description?

@hi019
Copy link
Collaborator

hi019 commented Aug 2, 2021

Yep, I'll update it later today. Forgot I had edit permissions :)

@hi019
Copy link
Collaborator

hi019 commented Aug 2, 2021

I've updated the checklist. Everything is done on this repository's side 👍

@jasonbosco
Copy link
Member Author

Thank you @hi019!

I was going to publish the package to packagist, but I wanted to confirm if the package/repo naming convention is ok. Looking at a couple of other Laravel Scout packages, they're all named laravel-scout-x-driver.

Eg:

Should we also change this repo/package name to laravel-scout-typesense-driver? Is there a common convention?

Also, is a Laravel Scout "engine" different from a Scout "driver"?

@hi019
Copy link
Collaborator

hi019 commented Aug 5, 2021

From what I understand after reading the Scout docs, an engine is the search provider itself, like Algolia or in this case Typesense. And a driver is what interfaces between the engine and Laravel code.

So, while I can't find any Scout naming conventions, it would make sense to name this repository & the package laravel-scout-typesense-driver.

@hi019
Copy link
Collaborator

hi019 commented Aug 5, 2021

@jasonbosco Can you wait until tomorrow to publish the package? I forgot to integrate the bulk delete API route I mentioned earlier in this Issue. It would likely be a small breaking change, so it would be easier to just put that change in the migration notes rather than publish a new major version

@jasonbosco
Copy link
Member Author

@hi019 Sounds good. I'll stand by.

And thank you for the research on naming the repo & package.

@hi019
Copy link
Collaborator

hi019 commented Aug 6, 2021

Unfortunately I won't have time to do this today, will complete it tomorrow or by monday

@hi019
Copy link
Collaborator

hi019 commented Aug 9, 2021

@jasonbosco Ready for release

@jasonbosco
Copy link
Member Author

@hi019 Thank you. I've published an RC version of the package here: https://packagist.org/packages/typesense/laravel-scout-typesense-driver

Could you give it a shot and let me know if it works ok?

@manavo
Copy link
Contributor

manavo commented Aug 11, 2021

I'm getting errors on the soft delete checks:

ErrorException: Undefined property: Typesense\LaravelTypesense\Engines\TypesenseEngine::$softDelete

@manavo
Copy link
Contributor

manavo commented Aug 11, 2021

I've made PR #5 to fix it

@manavo
Copy link
Contributor

manavo commented Aug 11, 2021

Bulk deleting isn't working for me either, I get an error like:

Typesense\Exceptions\ObjectNotFound: Could not find a filter field named `{TABLE_NAME}.id` in the schema

Where {TABLE_NAME} is the actual table name of course.

Since the table name leads to the collection, I'm guessing it maybe needs to be the unqualified name that we use? (without the table name?)

@manavo
Copy link
Contributor

manavo commented Aug 11, 2021

The issue is solved if I specifically define the getScoutKeyName() method:

    public function getScoutKeyName() {
        return 'id';
    }

But it would be good if this worked without needing to explicitly define that!

EDIT: Scrap that, it didn't work either!

@hi019
Copy link
Collaborator

hi019 commented Aug 11, 2021

I'll look into it later today @manavo . Do your models use the trait Laravel\Scout\Searchable? It defines a getScoutKeyName method.

@manavo
Copy link
Contributor

manavo commented Aug 11, 2021

Yep, it uses the trait, but the one in the trait does this:

    public function getScoutKeyName()
    {
        return $this->getQualifiedKeyName();
    }

And it makes me think that it is meant to be used in the Eloquent Query Builder, more than in the engines/drivers.

Even when I forced it to be just id instead of {TABLE_NAME}.id, I still get this error:

Could not find a filter field named `id` in the schema

So I'm not sure why that is. I'm guessing Typesense should let us filter by id?

@manavo
Copy link
Contributor

manavo commented Aug 11, 2021

I'm also running version 0.17.0 of Typesense, so I'll try and upgrade and see what happens.

@hi019
Copy link
Collaborator

hi019 commented Aug 11, 2021

@manavo Do you mind creating a new issue for the delete problem so that it's easier to track?

@manavo
Copy link
Contributor

manavo commented Aug 11, 2021

Same issue after the upgrade. No problem, I'll create a separate issue.

@manavo
Copy link
Contributor

manavo commented Aug 11, 2021

Moved to issue #6, and added some extra detail there @hi019

@jasonbosco
Copy link
Member Author

Thank you @hi019 and @AbdullahFaqeir for making this happen, and thank you to everyone who helped beta test this change!

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

No branches or pull requests

3 participants