search

jquery / sizzle

A sizzlin' hot selector engine.
https://sizzlejs.com
Other
6.29k stars 951 forks source link

Tests: Fix speed/index.html dependencies #419

Closed ameshkov closed 6 years ago

ameshkov commented 6 years ago

Currently, sizzle/speed/index.html does not work as there're two missing requirements: https://github.com/jquery/sizzle/blob/master/speed/speed.js#L6

mgol commented 6 years ago

This is a next version of PR #417.

There's no need to open a new PR; in the future please fix the branch from which you submitted the original PR and push to it (possibly if --force if you modified some commits); the PR will get updated automatically.

Opening a separate PR after feedback makes it harder to track a discussion.

ameshkov commented 6 years ago

As for the PR itself - how did you install dependencies? package.json should have been modified and it hasn't in this PR.

Uh, my bad - forgot to include it.

ameshkov commented 6 years ago

The repository needs to be at a state where running npm install && grunt npmcopy doesn't introduce any changes to the repo and all the files present in the npmcopy config should be in node_modules after running npm install.

I was confused by multiple changes occurred after grunt npmcopy -- there were old versions of qunit and requirejs -- different from what is declared in package.json. This time I've pushed everything.

mgol commented 6 years ago

@gibson042 I see package-lock.json is committed in this repo, contrary to us gitignoring it in other repos due to cross-platform npm problems. How do you want to proceed here?

ameshkov commented 6 years ago

package-lock is commited: https://github.com/jquery/sizzle/pull/419/commits/0226789bfc9276b7a3173b6212475b6638665590

mgol commented 6 years ago

Landed, sorry for the wait.