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

Eliminate dependencies on jquery, and underscore. #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gunn
Copy link

@gunn gunn commented Mar 3, 2016

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?

@gunn
Copy link
Author

gunn commented Mar 3, 2016

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>';

@arunoda
Copy link
Member

arunoda commented Mar 3, 2016

I'm also okay with removing underscore from the server as well. Go for it and add it to here as well.
Once you did that, just add a comment to notify me.

@gunn
Copy link
Author

gunn commented Mar 3, 2016

@arunoda: done. I haven't done any testing however. Can you tell me a good way to run the tests?

@arunoda
Copy link
Member

arunoda commented Mar 3, 2016

We have some tests which covers most of the cases.
Go to this directory and try this:

meteor test-packages ./ -p 9000

@gunn
Copy link
Author

gunn commented Mar 3, 2016

@arunoda Haha, previously I'd tried meteor test-packages ..
Okay, all passing now.

@@ -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/);
Copy link
Member

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?

Copy link
Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

@tomwasd
Copy link
Contributor

tomwasd commented Mar 3, 2016

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 querySelector but haven't used it in your PR. Did you have issues with it? I only ask as my implementation is like this:

  var dom = document.querySelector('script[type="text/inject-data"]');
  // .innerText fallback for IE8
  var injectedDataString =
    ((dom && (dom.textContent || dom.innerText)) || '').trim();
  InjectData._data = InjectData._decode(injectedDataString) || {};

And I'm hoping I don't have a bug lurking about that I'm not aware of!

Thanks :)

@gunn
Copy link
Author

gunn commented Mar 3, 2016

@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... 😛

@tomwasd
Copy link
Contributor

tomwasd commented Mar 3, 2016

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... 😮

@arunoda
Copy link
Member

arunoda commented Mar 3, 2016

@tomwasd Wow. A world without jQuery :)

@gunn I'll take the PR and do a release tomorrow.

@PEM--
Copy link

PEM-- commented Nov 17, 2016

Any news on this PR?

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