Skip to content
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.

Sign up for GitHub

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

Jump to bottom

pengines.js dependency on jQuery in browsers #44

Open
ridgeworks opened this issue Feb 22, 2019 · 2 comments
Open

pengines.js dependency on jQuery in browsers #44

ridgeworks opened this issue Feb 22, 2019 · 2 comments

Comments

@ridgeworks
Copy link
Contributor

pengines.js has a somewhat poorly documented dependency on jQuery's network support, i.e., any HTML document using pengines must also use jQuery. Furthermore pengines.js will fail to initialize unless jQuery has already been initialized (code from pengins.js initialization:

if (typeof window === 'undefined') {
  // Node.js
  module.exports = Pengine;
  var najax = require('najax');
  Pengine.network = najax;
  Pengine.network.ajax = najax;
} 
else {
  Pengine.network = $;
  // Browser
  $(window).on("beforeunload", function() {
    Pengine.destroy_all();
  });
}

While this can be work by loading scripts in the correct order in the <head> element, that's discouraged since it delays loading and rendering of the <body> resulting in a "poor user experience" (not my words). So web content designers are encouraged to load scripts asynchronously; in fact that's generally the only way when scripts are in the body element. And when that's done the current pengines.js may fail if $ is not yet defined.

The bottom line is that pengines.js is quite sensitive to the the host HTML document structure. I can see a couple of ways of improving the situation.

  1. Remove the dependency so JQuery doesn't have to be loaded at all. It's fairly straightforward just to use the native XHR already in browsers; as the najax developer says "jQuery ajax is stupid simple." And, unless jQuery is required for some other reason, it's one less script to load, which is generally a good thing. But this may entail a fair amount of work, so as an interim solution:

  2. Move the initialization of Pengine.network into the constructor. The chances are good that by the time the application itself is running and wants to use a Pengine, jQuery will be ready to go. And, if not, at least the application has an opportunity to recover, e.g., delay and try again.

Perhaps there's a better solution, but 2. is usable, simple, and safe.
@JanWielemaker
Copy link
Member

Makes sense to me. Please file a pull request (I guess for (2)).

@ridgeworks ridgeworks mentioned this issue Feb 22, 2019
Merged
@ridgeworks
Copy link
Contributor Author

I think I've done what's required but I'm not used to generating pull requests so let me now if I've screwed things up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@JanWielemaker @ridgeworks