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

Add config option to defer loading of Xray script #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SDRACK
Copy link

@SDRACK SDRACK commented Jun 17, 2019

When loading of jQuery is deferred, the Javascript Xray script loads ahead of this and loads a blank object.

This is a stripped down approach to allow defer loading the script, via a config option.

@mattbrictson
Copy link
Collaborator

Thanks for the PR. I don't think this is the right approach because local_config is for configuration that is developer-specific, not project-specific (e.g. their preferred code editor). Reusing local_config for the purpose of storing the defer setting isn't quite right because defer needs to be a project-based setting, not something a developer sets in their ~/.xrayconfig.

I like the idea of configuring the defer attribute though. Is there a different way to do it? Maybe the app could have an initializer like config/initializers/xray.rb and in that file you set an attribute on the Xray object? Just thinking out loud, not sure if that is ideal either.

@SDRACK
Copy link
Author

SDRACK commented Jun 26, 2019

Thanks for the reply @mattbrictson, and apologies for the delay.

You're entirely right on this point, and, with that in mind, I'd agree an initializer would be the way to go.

When I get the time I'll update to reflect this. Once again, thanks for the feedback 👍

@gfauredumont
Copy link

I'm interested in this feature ;)

@TylerRick
Copy link

TylerRick commented Feb 24, 2021

Technically, couldn't the middleware just check if there was a defer attribute in the script tag for jquery.js, and if so, copy its value to the injected script for xray.js?

I'm hesitant to even mention that, though, because now that I've thought more about it, I think the auto-adding the script feature is overrated, over-complicated, and error-prone (#100), and ought to just be removed (#110).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants