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

Fixed deprecated relatedTableAlias from Objection.js #43

Closed
wants to merge 4 commits into from

Conversation

marcpearson
Copy link

Hi

I made a modification to fix the error about relatedTableAlias. This function didn't exists anymore in objection.js.

Maybe you have a better way to fix this error !

@marcpearson
Copy link
Author

Oups ! I forgot to upgrade Objection.js dependency. Running with the currently latest (1.5.3) seem Ok

@coveralls
Copy link

coveralls commented Feb 8, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 2edf88f on marcpearson:master into 275bc04 on Vincit:master.

@kibertoad
Copy link
Collaborator

@marcpearson Thank you for your pull request! Could you please fix test failures?

@marcpearson
Copy link
Author

@kibertoad Excuse my english i'm a french speaker (Québec). Maybe i could help with that but i don't know how to set my computer to run your tests correctly. It seems i need to have postgreSQL and sqlite.

The errors seems to be related to others problems not because of my modification.

But if you explain me how to run tests i will take a look ...

Objection.js renamed build function to toKnexQuery from version 1.6.3
Upgrade dependencies and devDependencies to fix vulnerabilities
Upgrade minimal version of Objection to 1.6.3
@kibertoad
Copy link
Collaborator

@koskimas Does the original fix looks like a valid equivalent in modern Objection.js?

- Fixed breaking changes for objection 2.0.3
@vigzmv
Copy link
Contributor

vigzmv commented Dec 6, 2019

@marcpearson Hey, do you have any issue with the current released version of objection-find?
We have been using it in production for long, and it is working well
If there's an issue, let's try to fix it. Then we will patch it for Objection@v2

@marcpearson
Copy link
Author

To be honest, I have not tried your version for a while. The pull-request that I submitted a few months ago has never been merged and the search for objections does not work as before. It may be time to try again.

But I'm starting some new projects and I want to start with the Objection V2 branch. Until you support Objection V2, I will stay with my fork. After migrating to V2, I will try it and see if all my projects are successful in all my integration tests.

I will be happy to stay with the original objection-find but the development seems to slow down considerably

@vigzmv
Copy link
Contributor

vigzmv commented Dec 9, 2019

@marcpearson I already checked and tried your version, it works better. Can you please make a new PR with the fix commit and a migration to objection2 commit. That will help kibertoad to merge it.

I am also waiting for this to start using objection2

@kibertoad
Copy link
Collaborator

@marcpearson Any chance you can rebase this?

@marcpearson
Copy link
Author

HI @kibertoad

First, i'll be glad to help i very appreciate objection-find but because french is my native language i'm not sure to understand what i need to do. Secondly, i'm not an expert with Git/Github/Pull-request. So if you want to guide me i will do all the necessary things.

But before we go with further processing, some questions

  1. Why this initial pull-request was not merged ? Did i do something wrong ?
  2. Why objection-find still use objection.js V1.x.x and not the newly V2 ?

@vigzmv
Copy link
Contributor

vigzmv commented Mar 11, 2020

@marcpearson
We are not able to merge this PR, because it now has conflicts, you can either fix these conflicts or best,

  1. create a new branch from objection-find@master
  2. apply your changes and commit
  3. create a PR.

To answer your questions

  1. This PR couldn't merge because the tests were failing.
  2. Yes, we have to do the upgrade with fixing the breaking changes to use objection V2

@marcpearson
Copy link
Author

After rebasing my code with the latest commit from objection-find original repo i see that the bug #48 it still present. Searching with a field from HasOneRelation is broken. The fullColumName code is not working as before ... So i dont know what to do with this... my fork has already the fixed

I also see that the version in package-find.json is 2.0.0 but reported 1.6.5 in package-lock.json. My package-lock.json is updated after "npm install" but will this create a conflict

My fork is already patched to use objection V2+.

I can merge all the upstream changes im my fork and make some fixes but I'm not sure how to make the pull-request for all this minors problems ?

@marcpearson
Copy link
Author

After digging more thorouglhy in the code, the bug #48 is probably related to the fact that the tablename in all my objection model definition use the pattern «database.tablename» to be able to do joined queries againts different database in MySQL. The current _buildJoins method in FindQueryBuilder doesn't support tablename with a database part so i overload this method in an other part of my application (I forgot that !!!)

@kibertoad kibertoad mentioned this pull request Apr 3, 2020
@kibertoad
Copy link
Collaborator

@marcpearson I've applied fix to PropertyRef that you've done to master. However,

builder.eager(eager.value); ->
builder.withGraphFetched(eager.value);

change throws an eror due to method not existing. Is this a custom method that you introduced in your own fork?

@marcpearson
Copy link
Author

Eager method was deprecated and replace with «withGraphFetched» in objection version 2+. So be sure to use objection.js version 2+. Check in your node_modules if you have objection.js 2 installed. If not, refresh your node_modules installed version. Something like rm -rf node_modules and npm install

@kibertoad
Copy link
Collaborator

@marcpearson Could you please resolve the conflicts? Then this could be merged.

@marcpearson
Copy link
Author

It's no more necessary :)

@kibertoad
Copy link
Collaborator

@marcpearson I don't think we ever fixed this part :(. Why wouldn't this fix be useful anymore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants