cwrc / CWRC-WriterBase

The base class from which to create a CWRC-Writer XML editor.
GNU General Public License v2.0
14 stars 3 forks source link

Isolate library dependencies #25

Closed ajmacdonald closed 6 years ago

ajmacdonald commented 6 years ago

Make sure jQuery (and other libraries) don't pollute the global scope.

jchartrand commented 6 years ago

Is your plan to do something with this:

https://api.jquery.com/jquery.noconflict/

and potentially rewrite all references to $ in the cwrcwriter code to some other name like jqueryCwrc?

ajmacdonald commented 6 years ago

That's a potential solution. However, an additional complication is that there are libraries, e.g. bootstrap, that expect jQuery to be available as window.jQuery or window.$. So I'd need to modify the bootstrap build process.

jchartrand commented 6 years ago

ah, right - good point. What other options are you considering?

ajmacdonald commented 6 years ago

I'm hoping browserify-shim can be of some use but honestly this a pretty confounding problem to me.

jchartrand commented 6 years ago

From: https://community.canvaslms.com/thread/19727-jquery-and-bootstrap-conflict

"For anyone else looking for this solution, you basically have to add $.noConflict(true) at the end of your bootstrap.js file. That way Bootstrap will use the updated version of jQuery, and then release it so that Canvas can use the older version of jQuery."

Even if it works, it might not be ideal because we'd have to modify the bootstrap file directly, and it would only help with the bootstrap dependency (and not other libraries that depend on jquery), but maybe we could wrap the imports to all our dependencies somehow, and just release the jquery global at the end of our imports? I guess I still don't see how that would help if a library subsequently tries to get at jquery though global.$ or window.$ or window.jquery. However, maybe the libraries that depend on jquery all obtain and store a reference to jquery when loaded, and then use that subsequently, rather than looking for the global. wdyt?

jchartrand commented 6 years ago

Hmm, looking more closely at that guy's answer (again: https://community.canvaslms.com/thread/19727-jquery-and-bootstrap-conflict) it sounds like he uses browserify to generate a standalone javascript file (probably using the standalone switch https://github.com/browserify/browserify-handbook#standalone). Maybe that somehow, in combination with $.noConflict(true), helps isolate the jquery library to the packaged standalone.

It occurs to me, too, that generating a standalone cwrcwriter (using browserify standalone) might help with integration into islandora, since you could then avoid npm altogether and load the cwrcwriter code through a script tag in the html page. (but maybe you are already doing that)

jchartrand commented 6 years ago

Another similar thread:

https://stackoverflow.com/questions/19573074/is-there-a-way-to-use-bootstrap-3-0-plugins-with-jquery-noconflict

jchartrand commented 6 years ago

Do you know which libraries, other than bootstrap, we are using that depend on jquery? If we could make a list that might be helpful for sorting out other conflicts with jquery versions.

ajmacdonald commented 6 years ago

These: https://github.com/cwrc/CWRC-WriterBase/tree/master/src/js/lib/jquery/plugins And also: jquery-ui, jquery-layout, jstree,

jchartrand commented 6 years ago

More good stuff here about using noConflict, this time with jstree, but also more generally with jquery plugins:

https://stackoverflow.com/questions/33278178/how-to-use-2-versions-of-jquery-efficiently

And, as I guessed earlier, jquery plugins are indeed supposed to grab a reference to the jquery object at load time, and assign that to a to a reference to $ that is local to the plugin:

https://learn.jquery.com/plugins/basic-plugin-creation/#protecting-the-alias-and-adding-scope

all which makes it sound like we might well be able to use noConflict - we basically load our version of jquery, then load up all our stuff (including jstree, bootstrap, etc.), then call noConflict(true) to remove our reference from the global jquery variables, and revert back to the prior references to the old version of jquery.

We'd still of course have to make sure that we have no references to global.$ or window.$ etc. in our own code, and that we either scope $ locally or we rename our jquery, and all our references to it, to some other name, but that shouldn't be too bad since we have control over that.

wdyt?

ajmacdonald commented 6 years ago

We'd need someone (Brad?) to modify the Drupal code since it currently loads all of its js (including jQuery v1) before CWRC-Writer loads.

jchartrand commented 6 years ago

I think it is fine (in the sense that it will work, but not in the sense that it is a good idea given that the jquery docs say not to do it) to have the older version load first. Apparently jquery keeps track of the previously loaded version and reverts back to it when you call noConflict:

From:

https://api.jquery.com/jquery.noconflict/

Load two versions of jQuery (not recommended). Then, restore jQuery's globally scoped variables to the first loaded jQuery.

 <!doctype html><html lang="en"><head>  <meta charset="utf-8">  <title>jQuery.noConflict demo</title>  <script src="https://code.jquery.com/jquery-1.10.2.js"></script></head><body> <div id="log">  <h3>Before $.noConflict(true)</h3></div><script src="https://code.jquery.com/jquery-1.6.2.js"></script> <script>var $log = $( "#log" ); $log.append( "2nd loaded jQuery version ($): " + $.fn.jquery + "<br>" ); // Restore globally scoped jQuery variables to the first version loaded// (the newer version) jq162 = jQuery.noConflict( true ); $log.append( "<h3>After $.noConflict(true)</h3>" );$log.append( "1st loaded jQuery version ($): " + $.fn.jquery + "<br>" );$log.append( "2nd loaded jQuery version (jq162): " + jq162.fn.jquery + "<br>" );</script> </body></html>
jchartrand commented 6 years ago

And, from the same doc:


Old references of $ are saved during jQuery initialization; noConflict() simply restores them.

If for some reason two versions of jQuery are loaded (which is not recommended), calling $.noConflict( true ) from the second version will return the globally scoped jQuery variables to those of the first version.```
jchartrand commented 6 years ago

We might still have to use the browserify --standalone switch to generate our bundle, which is then loaded with the script tag, so that we can control when the noConflict is called. How are you using browserify with the islandora version?

ajmacdonald commented 6 years ago

I'm just using it the standard way: https://github.com/cwrc/Islandora-CWRC-Writer/blob/npmbuild/package.json#L35

jchartrand commented 6 years ago

How do you load the resulting app.js file into the browser? And how does the Islandora code get a reference to the cwrcWriter (if it does at all)?

ajmacdonald commented 6 years ago

app.js gets loaded by the Islandora Drupal code, which was the main issue here: https://github.com/cwrc/Islandora-CWRC-Writer/issues/15

And, we're setting globals: https://github.com/cwrc/Islandora-CWRC-Writer/blob/npmbuild/js/app.js https://github.com/cwrc/Islandora-CWRC-Writer/blob/npmbuild/js/islandora_cwrc_writer.js#L215

jchartrand commented 6 years ago

Ah, I see - the CWRCWriter sets itself in Drupal, rather than Drupal finding the CWRCWriter.

If it did make anything easier, we could do it the other way around: the browserify standalone switch would create a bundle that automatically exposes the CWRCWriter as a global variable that Drupal could find in the window object.

But, this wouldn’t affect the noConflict call. We just need to issue the noConflict call in a script, just after the app.js file is loaded in it’s own script tag (which is I think how it’s loaded)?

On Dec 15, 2017, at 2:17 PM, Andrew notifications@github.com wrote:

app.js gets loaded by the Islandora Drupal code, which was the main issue here: cwrc/Islandora-CWRC-Writer#15 https://github.com/cwrc/Islandora-CWRC-Writer/issues/15 And, we're setting globals: https://github.com/cwrc/Islandora-CWRC-Writer/blob/npmbuild/js/app.js https://github.com/cwrc/Islandora-CWRC-Writer/blob/npmbuild/js/app.js https://github.com/cwrc/Islandora-CWRC-Writer/blob/npmbuild/js/islandora_cwrc_writer.js#L215 https://github.com/cwrc/Islandora-CWRC-Writer/blob/npmbuild/js/islandora_cwrc_writer.js#L215 — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cwrc/CWRC-WriterBase/issues/25#issuecomment-352088947, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhZXfqI4rW5nFcOzT4tCeVwUUvoqo-iks5tAsXVgaJpZM4RDwI_.

ajmacdonald commented 6 years ago

Having a bit of success with this https://stackoverflow.com/a/36146810

ajmacdonald commented 6 years ago

I'm not sure the shim allows for standalone solution though. I'll look into it more next week.

jchartrand commented 6 years ago

Which shim do you mean? the browserify shim? I’m talking about the ‘standalone’ switch, as described here:

Forbes Lindesay — Standalone Browserify Builds https://www.google.ca/url?sa=t&rct=j&q=&esrc=s&source=web&cd=1&ved=0ahUKEwiFs5GqiY3YAhVH6WMKHXhMC_cQFggpMAA&url=http%3A%2F%2Fwww.forbeslindesay.co.uk%2Fpost%2F46324645400%2Fstandalone-browserify-builds&usg=AOvVaw2Fnm4X_tGIENBLtmZ1VB-u

or here:

https://github.com/browserify/browserify#usage https://github.com/browserify/browserify#usage

or here:

http://www.ryandaigle.com/a/expose-javascript-api-with-browserifyhttp://www.ryandaigle.com/a/expose-javascript-api-with-browserify http://www.ryandaigle.com/a/expose-javascript-api-with-browserifyhttp://www.ryandaigle.com/a/expose-javascript-api-with-browserify

On Dec 15, 2017, at 5:31 PM, Andrew notifications@github.com wrote:

I'm not sure the shim allows for standalone solution though. I'll look into it more next week.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cwrc/CWRC-WriterBase/issues/25#issuecomment-352128425, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhZXaphK1rOegoJiIE5UchfiW3D0AGqks5tAvNZgaJpZM4RDwI_.

jchartrand commented 6 years ago

Sorry, I hadn’t seen your prior comment.

On Dec 15, 2017, at 5:35 PM, James Chartrand jc.chartrand@gmail.com wrote:

Which shim do you mean? the browserify shim? I’m talking about the ‘standalone’ switch, as described here:

Forbes Lindesay — Standalone Browserify Builds https://www.google.ca/url?sa=t&rct=j&q=&esrc=s&source=web&cd=1&ved=0ahUKEwiFs5GqiY3YAhVH6WMKHXhMC_cQFggpMAA&url=http%3A%2F%2Fwww.forbeslindesay.co.uk%2Fpost%2F46324645400%2Fstandalone-browserify-builds&usg=AOvVaw2Fnm4X_tGIENBLtmZ1VB-u

or here:

https://github.com/browserify/browserify#usage https://github.com/browserify/browserify#usage

or here:

http://www.ryandaigle.com/a/expose-javascript-api-with-browserifyhttp://www.ryandaigle.com/a/expose-javascript-api-with-browserify http://www.ryandaigle.com/a/expose-javascript-api-with-browserifyhttp://www.ryandaigle.com/a/expose-javascript-api-with-browserify

On Dec 15, 2017, at 5:31 PM, Andrew <notifications@github.com mailto:notifications@github.com> wrote:

I'm not sure the shim allows for standalone solution though. I'll look into it more next week.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cwrc/CWRC-WriterBase/issues/25#issuecomment-352128425, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhZXaphK1rOegoJiIE5UchfiW3D0AGqks5tAvNZgaJpZM4RDwI_.

jchartrand commented 6 years ago

I read through that thread a couple of times but I just can’t figure out how it would apply in this case. Isn’t the point of the shim that it attaches a script loaded copy of Jquery to a global variable, and replaces any ‘require’ calls to jquery with a reference to the global variable?

How would it deal with conflicting versions of jquery?

On Dec 15, 2017, at 5:21 PM, Andrew notifications@github.com wrote:

Having a bit of success with this https://stackoverflow.com/a/36146810

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

ajmacdonald commented 6 years ago

@jchartrand you're right, it won't. I was heading down the wrong path. I've made some good progress on this today though.

ajmacdonald commented 6 years ago

17ffa89c70992ca74adc15a657aaaca0c26f51b1 a0e801edb62b622b9192226b4d1a3b53bb6271a8 https://github.com/cwrc/Islandora-CWRC-Writer/commit/8cd50116a6e222f9c685d6a5a3d2656f765a775e