-
Notifications
You must be signed in to change notification settings - Fork 27
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
Eliminate dependencies on jquery, and underscore. #6
base: master
Are you sure you want to change the base?
Conversation
Also, it would be nice if we could do away with underscore on the server. Could the template code be replaced with something as simple as this? var injectHtml = '<script type="text/inject-data">' + data + '</script>'; |
I'm also okay with removing underscore from the server as well. Go for it and add it to here as well. |
@arunoda: done. I haven't done any testing however. Can you tell me a good way to run the tests? |
We have some tests which covers most of the cases.
|
@arunoda Haha, previously I'd tried |
@@ -130,7 +132,8 @@ function getInjectedData(path) { | |||
var url = urlResolve(process.env.ROOT_URL, path); | |||
var res = HTTP.get(url); | |||
|
|||
var matched = res.content.match(/data">(.*)<\/script/); | |||
var matched = res.content.match(/data">(.*?)<\/script/); |
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.
What's the change here?
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.
The query makes it non-greedy. Previously it was matching additional tags. See http://regexr.com/3cu6c
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.
Okay.
Hey @gunn great shout on removing jQuery - I've had a local fork without jQuery for a while and kept meaning to do a PR! Just curious - you mentioned about about using
And I'm hoping I don't have a bug lurking about that I'm not aware of! Thanks :) |
@tomwasd no problems, just being cautious as I don't what browser versions we need to support. See http://caniuse.com/queryselector I was going to point out someone had already had trouble with .trim() in IE at #3 but... 😛 |
Thanks @gunn Ha I now use the lovely es5-shim package internally so I can be confident .trim() is present Next step is removing jQuery from Kadira... 😮 |
Any news on this PR? |
I don't think we should need all of jquery just for a couple of methods.
We could simplify this with
document.querySelector
if we don't care about IE <= 7.I don't think underscore is used on the client, so I've removed that dependency too.
Does this look good?