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.

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

Unpredictable JS load order creates undefined function type of errors #520

Closed
actual-saurabh opened this issue May 1, 2018 · 2 comments
Closed
Labels
good first issue If you're a first time contributor this is a good issue for you! hacktoberfest PRs for this issue count towards Hacktoberfest contributions! help wanted Looking for contributors to assist with this issue Type: Bug Bugs and errors

Comments

@actual-saurabh
Copy link
Contributor

actual-saurabh commented May 1, 2018

screen shot 2018-05-01 at 7 27 30 pm

This is the current order of the js files. Here's an ordered list for convenience:

  1. assets/js/llms-admin-tables.min.js
  2. assets/js/llms-admin.min.js
  3. assets/js/llms-ajax.min.js
  4. assets/select2/js/select2.min.js
  5. assets/js/llms-metabox-students.min.js
  6. assets/js/llms.min.js
  7. assets/js/vendor/topModal.js
  8. assets/js/llms-metabox-product.min.js
  9. assets/js/llms-metabox-instructors.min.js
  10. assets/js/llms-metabox-fields.min.js
  11. assets/js/llms-metaboxes.min.js

Often because the l10n object is defined in llms.min.js but used in scripts loaded before that where it throws undefined errors.

By using the $dependecy parameter of wp_enqueue_script() we could control this load order by adding scripts as dependencies of other scripts.

The final solution would involve deciding on a correct nested load order and then registering dependencies without explicitly enqueuing them.

This is because enqueueing the final script will surely enqueue scripts listed in the dependencies first.

@actual-saurabh actual-saurabh added Type: Bug Bugs and errors good first issue If you're a first time contributor this is a good issue for you! labels May 1, 2018
@thomasplevy
Copy link
Contributor

Related #146 & #484

@thomasplevy thomasplevy added this to the Scrub May 2018 milestone May 1, 2018
@thomasplevy thomasplevy added help wanted Looking for contributors to assist with this issue hacktoberfest PRs for this issue count towards Hacktoberfest contributions! language-php labels May 1, 2018
@actual-saurabh
Copy link
Contributor Author

actual-saurabh commented May 4, 2018

I had a chance to review this more closely.

We should remove enqueue_inline_script() and related methods in favour of wp_localize_script() calls. This way the l10n issues will go away if we attach the data to the main 'llms' script. This will ensure that all data is available to the script that needs them.

Second, make 'llms' a dependency of all the other scripts ensuring that all the lllms related objects are loaded and in place before any other scripts load.

This will halve the code in both the assets files and make a lot of issues go away. Also see: https://pippinsplugins.com/use-wp_localize_script-it-is-awesome/ for other benefits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue If you're a first time contributor this is a good issue for you! hacktoberfest PRs for this issue count towards Hacktoberfest contributions! help wanted Looking for contributors to assist with this issue Type: Bug Bugs and errors
Projects
None yet
Development

No branches or pull requests

2 participants