Feature request: Rewrite JS to not depend on jQuery, or have both version (jQuery-compatible and es6-compatible)

[pedrofurtado]

@nathanvda :+1:

[nathanvda]

Personally I still use jquery (and happy to), but I understand there is a trend to move away from it.

Also the callbacks and the insertion API now heavily depend on the presence of jquery, but it guess it would be a cool exercise to see how much backward compatibility we would loose when switching to a vanilla js version.

Also I like your suggestion to have both a jquery and vanilla js version.

[pedrofurtado]

@nathanvda Thanks for feedback!

It's a great option to provide the two versions, one with jQuery compatibility and one without it. Then, all kind of projects can use the gem without problems. I can help you to test and provide feedback, if you want :+1: :handshake:

[nathanvda]

To be honest: this does not rank very high on my to do list, but I will try to have a look.

[pedrofurtado]

@nathanvda No problems :+1: Anyway, thanks for your work in this great gem :clinking_glasses:

[Mirv]

So I've been poking through several things ... 7 of 9 javascript gems I checked stated about the same theme ...

There was a vanilla JS version, but it was buggy and slower ... so much so it was less work to just remove it & only use Jquery.

80% of what we do could be done in vanilla javascript, but that last 20% ... there is no better solution than Jquery.

---

I've been playing around with the idea of gutting the Jquery source code to only do the things Cocoon as a gem needs.

The file is 3,000 lines & I'm not done yet. Also, it will require maintenance to keep it updated - which is kind of a factor here.

I spent first half of the day trying to find a js library which was small enough to justify the work of switching off Jquery, but it really is the gold plated standard (In terms of comprehensiveness).

[thedanbob]

I agree that maintaining the current jquery-style API is nearly impossible, but someone using it in a non-jquery context wouldn't expect to be able to use the same API (for example, having association-insertion-method makes no sense unless you have the jquery traversal methods available). Here's a quick rewrite I did in ES6: https://gist.github.com/thedanbob/b48a9e20b57341ced21fdd9e2e0609b9

import Cocoon from 'cocoon'

new Cocoon('#tasks', {
  insertionNode: '.links',
  beforeInsert: function(content) { ... },
  // etc
})

Note that I wrote this very fast and haven't run it at all so it almost certainly doesn't work yet. I'll work on cleaning it up, if you like what you see I'll submit a pull request.

[Mirv]

Yea - I submitted a PR to divide out some of the #add_fields responsibility and make the path moving forward easier, as well as some minor cleanup earlier this week ... couldn't decide if I should leave it open.

[Mirv]

@danbob - I grabbed your version & hit it with es-linter - it probably mucked up some stuff but I also cleaned up a few other things and ran my suggestions from regex portion thru it too.

https://gist.github.com/Mirv/ee6b7bee14578fc20f489a316d34e97a

I am not having much luck with your class making syntax - so there's a ton of undefined in there...

    1:7   error  'Cocoon' is defined but never used                      no-unused-vars
   37:28  error  'findWrapperClass' is not defined                       no-undef
   76:3   error  Identifier 'regexp_bracer' is not in camel case         camelcase
   81:3   error  Identifier 'regexp_underscorer' is not in camel case    camelcase
   95:9   error  'newContents' is already defined                        no-redeclare
   95:23  error  'associationIdBuilder' is not defined                   no-undef
   96:9   error  'insertionNodeElem' is assigned a value but never used  no-unused-vars
   96:48  error  'insertionNode' is not defined                          no-undef
   96:63  error  'insertionTraversal' is not defined                     no-undef
  111:9   error  Identifier 'regexp_braced' is not in camel case         camelcase
  111:25  error  'regexp_bracer' is not defined                          no-undef
  112:9   error  Identifier 'regexp_underscored' is not in camel case    camelcase
  112:30  error  'regexp_underscorer' is not defined                     no-undef
  113:17  error  'createNewId' is not defined                            no-undef
  116:53  error  'newcontent_braced' is not defined                      no-undef
  119:10  error  'regexpBraced' is not defined                           no-undef
  120:7   error  Identifier 'regexp_braced' is not in camel case         camelcase
  120:23  error  'regexp_bracer' is not defined                          no-undef
  121:7   error  Identifier 'regexp_underscored' is not in camel case    camelcase
  121:28  error  'regexp_underscorer' is not defined                     no-undef
  148:24  error  'findWrapperClass' is not defined                       no-undef
  175:9   error  'toDelete' is not defined                               no-undef
  178:70  error  'toDelete' is not defined 

[thedanbob]

I got it working with the default form layout, haven't tested it beyond that yet: https://github.com/thedanbob/cocoon/blob/master/app/assets/javascripts/cocoon.es6.js

import Cocoon from 'cocoon.es6'

document.addEventListener('DOMContentLoaded', function() {
  if (document.getElementById('attachments'))
    new Cocoon('#attachments')
})

Edit: insertionFunc and before/after options work properly.

[Mirv]

That looks really good - I was just copy & pasting what we needed from jquery lib!

Also - sorry! I saw a bug in there when I got up this morning.

Thought to myself, "hopefully I got to work before he did!"

I had grabbed a copy I was playing with for ordering one of the assignments to shoot it thru some performance tests.

[thedanbob]

Thanks! I just committed a bunch of refactoring, if you have a chance to look through it let me know if you see anything broken. I also changed the API a bit and dropped support for optional data-* attributes in favor of explicit options in the constructor.

[nathanvda]

This looks really good! Thanks! :+1:

I will try to have a look in more detail somewhere this week/weekend :scream: 🙂

[Mirv]

@danbob

I was playing with this line 74 for awhile too, in an attempt to ...

• Use a descriptive name in place of a rather long conditional
• Encapsulate that conditional
• Remove the need for the empty ternary conditional & the '`' combo

Would be happy for a descriptive name of a function & just drop the code as is into the function body.

    var associations = `(?:${this.addFieldsLink.getAttribute('data-association')}|${this.addFieldsLink.getAttribute('data-associations')})`
    this.regexId = new RegExp(`_new_${associations}_(\\w*)`, 'g')
    this.regexParam = new RegExp(`\\[new_${associations}\\](.*?\\s)`, 'g')

Can we the right side of the assignment into a function?

addFieldPlural(association){
  var association = this.addFieldsLink.getAttribute('data-association');
  // assuming this one doesn't save an error into association
  if(!association)
    association =  this.addFieldsLink.getAttribute('data-associations')
  return association;
}

var associations = addFieldPlural
this.regexId = new RegExp(`_new_${associations}_(\\w*)`, 'g')
this.regexParam = new RegExp(`\\[new_${associations}\\](.*?\\s)`, 'g')

It's a few extra lines but it gets around some stuff that could be buggy later on ...

[thedanbob]

That's not a ternary conditional but rather a regex non-capturing group with alternation, to match either form of the association. Both are set by the link_to_add_association helper at the same time: https://github.com/nathanvda/cocoon/blob/master/lib/cocoon/view_helpers.rb#L92-L93 That's why the original code tested a regex against the template to find the right one. I'm guessing rails uses the singular version for has_one relations and plural for has_many? Not sure. In any case, I can't think of a situation where both could be present in the same nested fields template so it's probably safe as is. And if not, the link_to_add_association helper should be fixed to only set the correct one.

[Mirv]

not a ternary conditional but rather a regex non-capturing group with alternation

Just further showing that line isn't easily deciphered :)

Maybe just name the variable associationRegex to prep the reader if not changing right?

Side: Yea - has_one / has_many sounds legit

[thedanbob]

Good idea, when it comes to regex it never hurts to clarify :) I also switched the order of the two (plural first now) since this gem is probably used much more for has_many associations than has_one.

[persand]

Here's a vanilla JS thing that we use @kollegorna

https://github.com/kollegorna/cocoon-vanilla-js

I can't take any credit myself, all high fives should go to @osvaldasvalutis

[pedrofurtado]

@persand Do you use this is yours systems? Is a complete replacement for cocoon, or there is some feature of coocon that is not covered by this repo? 👍 thanks for share

@thedanbob Your gist is not available anymore. Could you publish it again? 🤝

[persand]

@pedrofurtado Yes, we use it in a lot of projects. It works well for us, but I'm not sure if it's a complete replacement. Ping @osvaldasvalutis

[osvaldasvalutis]

@pedrofurtado @persand yes, it's a complete solution.

[thedanbob]

@pedrofurtado I've updated my links with my new username. But I no longer need a vanilla JS version of cocoon and if I do in the future I'll probably just use @osvaldasvalutis's.

For reference, here's how I load the gem's JS in webpacker: https://gist.github.com/thedanbob/36e3d85802e6344d41c6ca9971a7e474

[dkniffin]

I am very interested in using this gem, but I do not want to depend upon jQuery anymore. I see there's a vanilla-js version of the JS side of this at https://github.com/kollegorna/cocoon-vanilla-js

It sounds like the only other thing preventing this gem from removing jQuery as a dependency is the callback API. It seems like you could replace the jQuery even emitting with native DOM events, using dispatchEvent. In fact, this is exactly what the fork linked above does. You could even add some logic in there that says "if jQuery is present, emit the jQuery event too", to not break backwards compatibility.

Edit: @nathanvda if I made a PR for this, would you accept it?