Skip to content

Eliminate dependencies on jquery, and underscore.#6

Open
gunn wants to merge 3 commits into
meteorhacks:masterfrom
gunn:master
Open

Eliminate dependencies on jquery, and underscore.#6
gunn wants to merge 3 commits into
meteorhacks:masterfrom
gunn:master

Conversation

@gunn

@gunn gunn commented Mar 3, 2016

Copy link
Copy Markdown

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

gunn commented Mar 3, 2016

Copy link
Copy Markdown
Author

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

arunoda commented Mar 3, 2016

Copy link
Copy Markdown
Member

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

gunn commented Mar 3, 2016

Copy link
Copy Markdown
Author

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

@arunoda

arunoda commented Mar 3, 2016

Copy link
Copy Markdown
Member

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

meteor test-packages ./ -p 9000

@gunn

gunn commented Mar 3, 2016

Copy link
Copy Markdown
Author

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

Comment thread tests/integration.js
var res = HTTP.get(url);

var matched = res.content.match(/data">(.*)<\/script/);
var matched = res.content.match(/data">(.*?)<\/script/);

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay.

@tomwasd

tomwasd commented Mar 3, 2016

Copy link
Copy Markdown
Contributor

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

gunn commented Mar 3, 2016

Copy link
Copy Markdown
Author

@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

tomwasd commented Mar 3, 2016

Copy link
Copy Markdown
Contributor

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

arunoda commented Mar 3, 2016

Copy link
Copy Markdown
Member

@tomwasd Wow. A world without jQuery :)

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

@PEM--

PEM-- commented Nov 17, 2016

Copy link
Copy Markdown

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