Rewrite CoffeeScript components in ES6 by javierjulio · Pull Request #5081 · activeadmin/activeadmin

javierjulio · 2017-07-14T01:49:01Z

This adds the sprockets-es6 gem so we can write our JS as ES6 and remove the coffeescript dependency. While converting the CoffeeScript to ES6 is easy enough, I wanted to add a quick proof of concept for replacing jQuery UI widgets with a standalone ones where possible. I've started with tabs as that was the easiest but also a big win. The tabs in jQuery UI are well over 900+ lines of code whereas this is just 200 and the same functionality. I challenged myself today to get this done in an hour and was successful.

I've since removed the 2 commits that had converted the tabs from jQuery UI to something that is standalone and working from this PR. I created a new branch from this one called add-tabs-from-bootstrap-v4 where I left that change in place to get it later. I will continue converting to ES6 in this PR.

Closes #5050

javierjulio · 2017-07-28T16:54:23Z

I'm just going to remove initializers/batch_actions.js.coffee from here anyway since its only for an extreme case and has no need. This can and should be handled in CSS which is addressed in #3862 (where the file was also removed). Easier to just deal with it now since it doesn't break any functionality.

javierjulio · 2017-07-28T17:09:05Z

This is ready for review if you have the time. I've kept the commits focused so open each if you want to see only the changes for that. Should be easier to review and see what's changing. Thanks!

After merging this in, I will start replacing the $.widget.bridge calls that way we rely less on jQuery UI. This is easy to implement internally as I've done jQuery plugins before. And from there further changes to slowly start removing the jQuery UI dependency.

javierjulio · 2017-07-28T17:21:22Z

Once I removed coffeescript (coffee-rails gem), running the test app in a browser after starting the local server wasn't working, but it seems that my test app stuck around even after clearing out the test app with rm -rf spec/rails && bundle update. Either way since it was still there it had the original active_admin.js.coffee file so all I had to do was update that and it works fine. The tests are passing.

varyonic

Looks reasonable to me.

javierjulio · 2017-07-31T21:08:54Z

@varyonic thanks! You reviewed, but not approved. Are you ok with me merging in? Anyone else feel like putting it in a review? I would like to get this in very soon as I've already started updating components. Its going really well! I'm updating the PerPage component to demonstrate what it would be like without the jQuery UI widget bridge and also not needing to attach Turbolinks events either (we'll do event delegation through document instead which solves that problem). Working great so far!

Tested that the modal receives focus, not the first input within it. Updated the serializeArray method to be functional by using reduce instead.

Existing functionality preserved

Removed duplicate code for the clear filters functionality, so now we filter out the params as we want first, and then use it to adjust what the location. All original functionality has been preserved. Fix regex

Tested all functionality and its maintained for both table checkbox toggler and the checkbox toggler (this is used by another index page type in conjunction with `resource_selection_cell`).

Tested all functionality and works great even with existing or no query params. I adjusted the regex to account for the preceding “?” character in the query string rather than just doing `substring(1)` at the end of `window.location.search` which isn’t clear at first why its there. I preferred to keep everything in the regex as it makes sense since the character is part of the string.

Tested all functionality and it works great, even checking every checkbox on an index page. Tested with both default index table and when used on a custom index type with resource_selection_cell.

Tested functionality and works well. I left the option method in place because we use the plugin method with an argument from the batch actions code to enable/disable the drop down based on what has and hasn’t been checked.

Tested and working fine with all the various options shown in the example in the docs. Opening and closing the dialog works great too.

Tested on the new post form by adding tags and saving, then moving and saving, and deleting as well. Everything worked as it did before.

This doesn’t add any behavior, only modifies appearance for an extreme case. No actual functionality is broken because of this and the presentation part is dealt in activeadmin#3862 as CSS where it should be.

I’m keeping the manifest file as `.js` since there is no need to have it be `.es6` so its very much on purpose.

javierjulio · 2017-08-02T19:17:44Z

The build is green but for some reason codecov failed. Its not able to compare commits. I had rebased and edited a commit so that could be why.

varyonic

Would like to see more approvals, but yes, I'm OK with this.

faucct · 2017-11-29T09:04:10Z

This change makes the phantomjs features testing modal windows crash with ReferenceError: Can't find variable: Symbol. Maybe you should include some polyfills with it.

faucct · 2017-11-29T10:29:07Z

Simply requiring the babel/polyfill from https://github.com/babel/ruby-babel-transpiler/blob/6878f6edb4e717066bbe344c25b05055cd2d914f/babel-source.gemspec.erb#L14 does the thing:

# active_admin.js.coffee
#= require babel/polyfill
#= require active_admin/base

javierjulio · 2017-11-29T15:19:41Z

@faucct since you mentioned phantomjs, I know it was replaced recently with headless chrome so it could be you are running tests on an older version. Best we moved off of phantomjs.

faucct · 2017-11-29T16:22:09Z

I don't mean activeadmin repository feature tests, but our own. Still, the polyfills should be included to avoid breaking compatibility with browsers not having es6 features.

javierjulio · 2017-11-29T16:23:34Z

So you're running the tests against the ES6 code rather than what it compiles too? When it tested against CoffeeScript, did it not first convert it to JS or were the tests running against the CoffeeScript code itself?

faucct · 2017-11-29T16:36:01Z

Of course the tests are running against the compiled code, but the compiled code expects your browser have some es6 constants defined (Symbol in this case). You can see the compiled code for yourself and find that this line has transformed into something that uses global Symbol variable which is not present at all browsers - http://kangax.github.io/compat-table/es6/#test-Symbol. That's what polyfills are for.

javierjulio · 2017-11-29T16:46:01Z

I ask because I'm not aware if they are so its not obvious to me. Do you know why its expecting to have an ES6 constant defined? I thought by having this here it would convert but keep the code in ES5 and not rely on something that is ES6 based in the compiled code. Do you know why its using the Symbol variable? Do you have a suggestion on rewriting that line so it doesn't use the Symbol variable perhaps? Yes I know what polyfills are for. This is unexpected so its not something you can expect me or others to be aware of since it wasn't even clear what it was when you described the issue. There is no need for a condescending tone. I'm trying to help but first I need to understand what's wrong.

faucct · 2017-11-29T17:04:07Z

Sorry, didn't mean to be offensive: the ES6 is a new thing to me too and I don't understand the reasons for a lot of things there. You should use for ... in ... instead of for ... of ... for iterating over object properties. Then you won't need the Symbol constant defined.

javierjulio · 2017-11-29T17:12:36Z

I'm sorry as ES6 is also new to me. I love some of the improvements but overall I don't know ES6 very well.

I'm glad you pointed that out, because why I asked several questions before is that I used a CoffeeScript to ES6 converter which now addresses your concern as to why it might seem odd it was one way rather than another.

Now we are getting somewhere and are understanding each other. 👍🏻

A fair amount of JS I rewrote because it was simple enough. The portion you linked too though is so gnarly. 😱 I dug through many commits to understand why it was written the way it was. I gave it a shot but had trouble wrapping my head around it from what I can last remember. I did test the change though so I know it works. I asked if you have thoughts on how that line can be rewritten because I passed it through a converter.

It sounds like we should be using for in and hopefully that will solve both issues, no need for a polyfill which I feel is ideal and those feature tests should run successfully. Can you make that ES6 change, test it locally and let me know how it works for you? I want to be sure the change fixes the feature tests too. I can give it a try (I'm sure other contributors will comment) if you submit a PR with that change. Thanks!

faucct · 2017-11-29T17:15:16Z

Okay, I will have the PR tomorrow.

javierjulio mentioned this pull request Jul 26, 2017

Interest in moving away from coffeescript? #5101

Closed

javierjulio force-pushed the add-sprockets-es6 branch 2 times, most recently from 3bce0e2 to 5a9f325 Compare July 27, 2017 00:19

javierjulio added this to the 2.0.0 milestone Jul 27, 2017

javierjulio self-assigned this Jul 27, 2017

javierjulio mentioned this pull request Jul 27, 2017

Remove unused flash JS component #5107

Merged

javierjulio force-pushed the add-sprockets-es6 branch 2 times, most recently from 8d91f42 to 6391981 Compare July 27, 2017 21:40

javierjulio added the ready label Jul 27, 2017

javierjulio requested review from Fivell, deivid-rodriguez and varyonic July 27, 2017 21:53

javierjulio changed the title ~~WIP: Rewrite JS as ES6~~ Rewrite JS as ES6 Jul 28, 2017

javierjulio changed the title ~~Rewrite JS as ES6~~ Rewrite CoffeeScript components in ES6 using sprockets-es6 Jul 28, 2017

javierjulio changed the title ~~Rewrite CoffeeScript components in ES6 using sprockets-es6~~ Rewrite CoffeeScript components in ES6 Jul 28, 2017

javierjulio mentioned this pull request Jul 28, 2017

Move all jruby travis builds to allow_failures section #5112

Merged

varyonic reviewed Jul 29, 2017

View reviewed changes

varyonic force-pushed the add-sprockets-es6 branch from c0ee141 to 510c704 Compare July 29, 2017 20:43

javierjulio force-pushed the add-sprockets-es6 branch 2 times, most recently from 6588453 to 24c97f1 Compare July 31, 2017 14:18

javierjulio added 6 commits August 2, 2017 13:49

Add sprockets-es6 dependency

e10bbf9

Update base to es6

06fa3df

Update tabs to es6

47d42f9

Update jQuery extensions to es6

8d3d751

Tested that the modal receives focus, not the first input within it. Updated the serializeArray method to be functional by using reduce instead.

Update datepicker to es6

63f9546

Existing functionality preserved

Update filters to es6 and simplify clear filters

e0d35cc

Removed duplicate code for the clear filters functionality, so now we filter out the params as we want first, and then use it to adjust what the location. All original functionality has been preserved. Fix regex

javierjulio added 8 commits August 2, 2017 13:57

Update checkbox toggler components as es6

26cf19e

Tested all functionality and its maintained for both table checkbox toggler and the checkbox toggler (this is used by another index page type in conjunction with `resource_selection_cell`).

Update batch actions to es6

823ec80

Tested all functionality and it works great, even checking every checkbox on an index page. Tested with both default index table and when used on a custom index type with resource_selection_cell.

Update dropdown menu to es6

12818c8

Tested functionality and works well. I left the option method in place because we use the plugin method with an argument from the batch actions code to enable/disable the drop down based on what has and hasn’t been checked.

Update modal dialog to es6

166c314

Tested and working fine with all the various options shown in the example in the docs. Opening and closing the dialog works great too.

Update has_many to es6

e6cc36b

Tested on the new post form by adding tags and saving, then moving and saving, and deleting as well. Everything worked as it did before.

Remove batch action initializer

937e398

This doesn’t add any behavior, only modifies appearance for an extreme case. No actual functionality is broken because of this and the presentation part is dealt in activeadmin#3862 as CSS where it should be.

Remove coffee-rails dependency

7ec59ff

I’m keeping the manifest file as `.js` since there is no need to have it be `.es6` so its very much on purpose.

javierjulio force-pushed the add-sprockets-es6 branch from 24c97f1 to 7ec59ff Compare August 2, 2017 18:03

varyonic approved these changes Aug 3, 2017

View reviewed changes

javierjulio merged commit be47c41 into activeadmin:master Aug 3, 2017

javierjulio deleted the add-sprockets-es6 branch August 3, 2017 21:14

faucct mentioned this pull request Nov 30, 2017

Modal dialog js library should not depend on ES6 features #5257

Merged

Uh oh!

Conversation

javierjulio commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javierjulio commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javierjulio commented Jul 28, 2017

Uh oh!

javierjulio commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varyonic left a comment

Choose a reason for hiding this comment

Uh oh!

javierjulio commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javierjulio commented Aug 2, 2017

Uh oh!

varyonic left a comment

Choose a reason for hiding this comment

Uh oh!

faucct commented Nov 29, 2017

Uh oh!

faucct commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javierjulio commented Nov 29, 2017

Uh oh!

faucct commented Nov 29, 2017

Uh oh!

javierjulio commented Nov 29, 2017

Uh oh!

faucct commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javierjulio commented Nov 29, 2017

Uh oh!

faucct commented Nov 29, 2017

Uh oh!

javierjulio commented Nov 29, 2017

Uh oh!

faucct commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

javierjulio commented Jul 14, 2017 •

edited

Loading

javierjulio commented Jul 28, 2017 •

edited

Loading

javierjulio commented Jul 28, 2017 •

edited

Loading

javierjulio commented Jul 31, 2017 •

edited

Loading

faucct commented Nov 29, 2017 •

edited

Loading

faucct commented Nov 29, 2017 •

edited

Loading