[WIP] IntegratesQueryBuilder mixin #3

hivokas · 2019-02-13T22:11:15Z

This PR adds IntegratesQueryBuilder mixin which integrates https://github.com/coderello/js-query-builder

Example of use:

Article.all({ builder: query => query.filter('name', 'how to').sort('id').page(3) })

hivokas · 2019-02-13T22:27:55Z

Do you have any ideas on how to cover that by tests?
I'm a little bit confused because js-query-builder is an implicit dependency (not listed in package.json)

lorisleiva

Hi 👋

Thanks for this very cool PR.

I've written a few comments on your code. Regarding the testing, I'm not sure how to mock a node module. Maybe this article could help you get started.

I hope this helps.

lorisleiva · 2019-02-14T17:36:56Z

src/IntegratesQueryBuilder.js

+{
+    beforeRequest(request) {
+        if (superclass.hasOwnProperty('beforeRequest')) {
+            superclass.beforeRequest(request)


Since we assume in the rest of the code that this mixin depends on MakesRequests, there is no need to check for the existence of beforeRequest. We can simply do:

request = super.beforeRequest(request)

lorisleiva · 2019-02-14T17:40:02Z

src/IntegratesQueryBuilder.js

+        let queryBuilderModule
+
+        try {
+            queryBuilderModule = require('js-query-builder')


Good, that seems to be the way to define optional dependencies. We just need to set it up in the externals webpack options and according to this issue it should recognise this as an optional external.

I'm not sure if it is necessary to define it as external dependency somewhere.

require() works like a charm.

Okay cool, so you don't have any errors when running npm run build?

I haven't tried npm run build yet.
But I have no errors with npm run test both with and without js-query-builder being installed.

lorisleiva · 2019-02-14T17:50:17Z

src/IntegratesQueryBuilder.js

+
+        request.builder(builder)
+
+        request.query = builder


This is a nice integration of js-query-builder but I feel that we could probably do better. Something more laravel-like.

let articles = Articles.query() .filter('age', 20) .sort('-created_at', 'name') .get()

We'd simply need to create a static query method that would return a new class QueryBuilder. This class needs to keep track of the model and have a get methods to send the accumulated request.

Note that both this approach and the current approach can be used together.

Anyway feel free to discard this if you'd rather leave things like this.

I like your idea 👍

hivokas · 2019-02-16T05:19:48Z

Thanks for the comments!
I will update PR soon.

hivokas · 2019-02-16T21:15:41Z

I've updated the code and made it more laravel-like.

lorisleiva · 2019-02-17T11:04:50Z

I really like your changes. Returning the builder directly with an extra get function on it is a brilliant idea. Would you mind writing a few tests before I can merge this?

hivokas · 2019-02-17T11:57:37Z

Thanks!

Sure, I'll write a few tests.
I think mock-require could help me to mock js-query-builder module. I'll try it asap.

hivokas · 2019-02-19T19:34:22Z

I've added the tests. But, unfortunately, have a problem with optionality of js-query-builder.

hivokas · 2019-02-19T19:37:03Z

WARNING in ./src/IntegratesQueryBuilder.js
Module not found: Error: Can't resolve 'js-query-builder' in '/home/hivokas/development/projects/javel/src'
 @ ./src/IntegratesQueryBuilder.js
 @ ./src/index.js

hivokas · 2019-02-19T19:43:04Z

Adding js-query-builder to the externals solves the issue for javel build.

externals: {
    mixwith: 'mixwith',
    'js-query-builder': 'js-query-builder',
},

But when I install javel to another project and try to build it, I receive a warning similar to mentioned in the previous comment.

hivokas · 2019-02-19T19:45:24Z

Do you have any ideas on how to beat this problem?

lorisleiva · 2019-02-21T08:15:59Z

Hi @hivokas thanks for writing those tests. Unfortunately I am really struggling to find any literature regarding optional externals with webpack. Could you copy/paste the warning that you get when installing Javel to another project?

hivokas · 2019-02-22T15:43:28Z

                Chunks             Chunk Names
/assets/dashboard/app.css   237 KiB  /assets/dashboard/app  [emitted]  /assets/dashboard/app
 /assets/dashboard/app.js  1.86 MiB  /assets/dashboard/app  [emitted]  /assets/dashboard/app
     /assets/main/app.css   396 KiB  /assets/dashboard/app  [emitted]  /assets/dashboard/app

WARNING in ./node_modules/javel/dist/javel.js
Module not found: Error: Can't resolve 'js-query-builder' in '/home/hivokas/development/projects/blog/node_modules/javel/dist'
 @ ./node_modules/javel/dist/javel.js
 @ ./resources/assets/dashboard/js/models/BaseModel.js
 @ ./resources/assets/dashboard/js/models/Article.js
 @ ./node_modules/babel-loader/lib??ref--4-0!./node_modules/vue-loader/lib??vue-loader-options!./resources/assets/dashboard/js/components/App.vue?vue&type=script&lang=js&
 @ ./resources/assets/dashboard/js/components/App.vue?vue&type=script&lang=js&
 @ ./resources/assets/dashboard/js/components/App.vue
 @ ./resources/assets/dashboard/js/app.js
 @ multi ./resources/assets/dashboard/js/app.js ./resources/assets/dashboard/css/app.scss ./resources/assets/main/css/app.scss
Done in 12.60s.

lorisleiva · 2019-02-27T14:49:40Z

@hivokas That is very annoying, I wrote a reply on an existing webpack issue to see if we're missing anything. Otherwise, I might have to consider using a different bundling solution.

hivokas · 2019-02-27T20:54:52Z

I have updated the pull request. What do you think about such workaround?

lorisleiva · 2019-02-28T14:45:00Z

Thanks for your work on this.

There is something I'm not sure to understand.

Is this Module not found warning only occurring because of the testing?
If we did not need to test this module at all, and did not include the dependency as an external, would we still have this warning?

Because if this is the case, then I think you work around makes sense but I'd like the "fake module registration" to belongs in the test folder whether than the src folder.

hivokas · 2019-02-28T21:25:14Z

Module not found error occurs because of require('js-query-builder') in IntegratesQueryBuilder.

hivokas · 2019-02-28T21:32:00Z

What do you mean by "fake module registration"?
It is not fake module registration.

Let me explain my idea:

If user wants to use IntegratesQueryBuilder he will need to:

install js-query-builder into his project;
register it using Javel class:

import * as JsQueryBuilder from 'js-query-builder'
import { Javel } from 'javel'

Javel.registerOptionalModule('js-query-builder', JsQueryBuilder)

hivokas · 2019-02-28T21:37:37Z

This way I'm avoiding require() use and the warning previously caused by it.

lorisleiva

Oh I see. Thank you for your explanation.

Now I understand why you've decided to mock your library in the tests.

I think it's a good idea but I just have a few comments regarding the implementation that I've written as a review.

lorisleiva · 2019-03-06T17:08:09Z

package.json

@@ -41,6 +41,7 @@
    "babel-plugin-webpack-alias-7": "^0.1.1",
    "eslint": "^5.12.1",
    "eslint-loader": "^2.1.1",
+    "mock-require": "^3.0.3",


I don't think we need this dependency anymore, right?

lorisleiva · 2019-03-06T17:22:15Z

src/IntegratesQueryBuilder.js

@@ -0,0 +1,43 @@
+import { Mixin } from 'mixwith'
+import Javel from '@/Javel'


I'd rather register the modules a little bit differently.

Instead of exposing a global Javel class...

import { Javel } from 'javel' Javel.registerOptionalModule(name, dependency) let dependency = Javel.resolveOptionalModule(name)

... I'd rather expose the functions directly

import { registerModule, resolveModule } from 'javel' registerModule(name, dependency) let dependency = resolveModule(name)

That way we can even rename the Javel class into something more specific like ModuleRegistration.

What do you think?

I like your idea.

lorisleiva · 2019-03-06T17:24:37Z

src/IntegratesQueryBuilder.js

+        try {
+            queryBuilderModule = Javel.resolveOptionalModule('js-query-builder')
+        } catch (e) {
+            throw new Error('IntegratesQueryBuilder mixin requires optional "js-query-builder" module to be registered.')


I don't think the try/catch here is necessary anymore since the resolveOptionalModule method already has it's own try/catch.

I've wrapped resolving of the module with try/catch in order to make an exception message more sensible.

Okay makes sense. 🙂

hivokas · 2019-03-06T20:24:11Z

I've updated the pull request according to your review.

lorisleiva · 2019-03-11T10:51:22Z

Thank you very much this PR and for your work on finding out how to register optional modules! Before I create a new release, can you please confirm that this PR is completely transparent to users that do not use the js-query-builder dependency?

Thanks again! 🍺

hivokas · 2019-03-11T15:17:41Z

Do we need kinda readme for this mixin?

hivokas · 2019-03-13T09:09:48Z

@lorisleiva

lorisleiva · 2019-03-13T09:44:00Z

Hi, yes I think we would need to update the documentation to explain this new module registration system. I see it as:

One small section at the end of the README explaining how to add mixins that have dependencies in Javel.
For each mixins that have external dependencies, add in their documentation page how to register the dependency itself.

What do you think?

hivokas · 2019-03-14T07:33:18Z

@lorisleiva, yeah, I agree with you. Who will work on it: you or me?

lorisleiva · 2019-03-14T10:42:46Z

Hi, I'm more than happy to do it. I just think that once I've written the main structure of the second bulletpoint, maybe you could document some of the features of your js-query-builder library.

I won't be able to do that just now but I'll open an issue to make sure it's in my pipeline. 👍

hivokas · 2019-03-14T13:28:25Z

Deal 🕶️
Let me know when I can start my part (you can mention me in #6).

IntegratesQueryBuilder mixin

ad3aa2c

lorisleiva reviewed Feb 14, 2019

View reviewed changes

improve integration of js-query-builder (make it more laravel-like)

dcbb79c

fixes IntegratesQueryBuilder mixin, adds tests

f0fb388

mock-js-query-builder

d48929e

lorisleiva mentioned this pull request Feb 27, 2019

optional dependencies when using es6 import/export webpack/webpack#7713

Closed

adds Javel class with ability to register an optional modules

d802a02

lorisleiva reviewed Mar 6, 2019

View reviewed changes

Ilya Sakovich added 3 commits March 6, 2019 22:07

package-lock.json revert

48e746f

removes mock-require

9758ec2

ModuleRegistrar class, functions

5f76680

lorisleiva merged commit fc48d7c into lorisleiva:master Mar 11, 2019

		@@ -0,0 +1,43 @@
		import { Mixin } from 'mixwith'
		import Javel from '@/Javel'

[WIP] IntegratesQueryBuilder mixin #3

[WIP] IntegratesQueryBuilder mixin #3

Conversation

hivokas commented Feb 13, 2019

hivokas commented Feb 13, 2019

lorisleiva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hivokas commented Feb 16, 2019

hivokas commented Feb 16, 2019

lorisleiva commented Feb 17, 2019

hivokas commented Feb 17, 2019

hivokas commented Feb 19, 2019

hivokas commented Feb 19, 2019

hivokas commented Feb 19, 2019

hivokas commented Feb 19, 2019

lorisleiva commented Feb 21, 2019

hivokas commented Feb 22, 2019

lorisleiva commented Feb 27, 2019

hivokas commented Feb 27, 2019

lorisleiva commented Feb 28, 2019

hivokas commented Feb 28, 2019

hivokas commented Feb 28, 2019

hivokas commented Feb 28, 2019

lorisleiva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hivokas commented Mar 6, 2019

lorisleiva commented Mar 11, 2019

hivokas commented Mar 11, 2019

hivokas commented Mar 13, 2019

lorisleiva commented Mar 13, 2019

hivokas commented Mar 14, 2019

lorisleiva commented Mar 14, 2019

hivokas commented Mar 14, 2019 • edited

hivokas commented Mar 14, 2019 •

edited