-
Notifications
You must be signed in to change notification settings - Fork 106
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
Use esbuild instead of webpacker #1957
Conversation
render: function (data, type, row, meta) { | ||
var api = new $.fn.dataTable.Api(meta.settings); | ||
let selected = api.rows(meta.row, { selected: true }).count() > 0; | ||
return `<input type="checkbox" ${selected ? 'checked' : ''}> ${selected ? 'checked' : ''}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is ever used or is useful. plus it adds the text checked
to the innerHtml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
files app test fine with this removed, so we should be good.
// FIXME: confim modals don't work in esbuild. | ||
// import 'data-confirm-modal'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may actually be OK because it's buggy: #665.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments.
window.$ = $; | ||
import jQuery from 'jquery'; | ||
import 'jquery-ujs'; | ||
import datatables from 'datatables.net'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to decouple. Why move the datatables here? if there is a piece of functionality that needs datatable access, shouldn't we do the same as we did in the files app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought, by adding datatables import here, we're loading datatables even on pages that do not use datatables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but jquery plugins don't play nice when jquery has a conflict (i.e., some other definition of jquery). #1846 is an example of this. Indeed - tooltip is a global jquery plugin. Along with poppers that we have installed everywhere.
I figure if we buy in on jquery, we buy in on it's plugins too.
I can try to fix this but it didn't work easily so i'll bet we have to use jquery's noConflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to fix. I was just asking the question.
|
||
import '../images/OpenOnDemand_powered_by_RGB.svg'; | ||
import '../images/OpenOnDemand_stack_RGB.svg'; | ||
datatables(window, jQuery); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more coupling? Now, we're having to pass window and jQuery to the datatable. Seems counter-productive to the goal of decoupling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deserves a comment which I'll give it, but this is here to initialize these plugins. They don't directly initialize just by being imported (because of javascript toolchain compilations and how old it is and how it's being invoked).
So I had to initialize them to tie them to this jquery otherwise they never invoke.
render: function (data, type, row, meta) { | ||
var api = new $.fn.dataTable.Api(meta.settings); | ||
let selected = api.rows(meta.row, { selected: true }).count() > 0; | ||
return `<input type="checkbox" ${selected ? 'checked' : ''}> ${selected ? 'checked' : ''}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
files app test fine with this removed, so we should be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #1825 to move to
esbuild
instead ofwebpacker
.I think it offers a few things
I believe this is ready for review. There is one drawback here where webpacker would automatically rebuild the assets whenever you modified them. This is no longer the case. Rails provides a simple
foreman
mechanism, but I haven't gotten it to work within OOD, so that's something we'll need to look into. I've providedbin/recompile_js
as a helper to do what's required and tests automatically do what's required.┆Issue is synchronized with this Asana task by Unito