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

Support for RequireJS #8

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

Conversation

bitter
Copy link

@bitter bitter commented Oct 11, 2010

Hi!

I made some changes which makes the htmlparser compatible with RequireJS and I also added two test runners for that environment. All I had to do was remove the detection of '__filename' and '__dirname'. Those global variables are node specific and not part of CommonJS.

I did not do a new minified version because I don't know which minification tool you use.

Anyhow, pull the change if you like it. If not, no biggy.

specific variables and not part of the CommonJS spec.

Also added test for running Node with RequireJS:
http://requirejs.org/
@tautologistics
Copy link
Owner

Will pull changes for 1.8

@bitter
Copy link
Author

bitter commented Jan 7, 2011

Great! Thanks for the htmlparser. It's much faster than the browser when you need to traverse html, the stupid browser always seem to load images and other bogus resources.

@tautologistics
Copy link
Owner

1.8 is coming together finally. I merged your changes into a bitter-master branch (https://github.com/tautologistics/node-htmlparser/tree/bitter-master) but I am having some issues getting tests to pass:

Total tests: 22
Failed tests: 0

node.js:116
    throw e; // process.nextTick error, or 'error' event on first tick
    ^
TypeError: Cannot read property 'type' of null
at runtests.rjs.js:39:36
at Function.execCb (require.js:1528:25)
at execManager (require.js:505:27)
at Object.onDep (require.js:579:33)
at execManager (require.js:536:45)
at main (require.js:655:17)
at callDefMain (require.js:668:18)
at Object.completeLoad (require.js:1181:21)
at Function.load (node.js:75:17)
at loadPaused (require.js:916:21)

One minor problem is that the summary (Total tests/Failed tests) is printed right away instead of after the async tests are run. The main issue is that the inner require() that loads the tests is passing a null module parameter to the callback. I do not have enough experience with RequireJS or the node shim to dig into this quicklu. Do you have time to ake a look?

@bitter
Copy link
Author

bitter commented Mar 7, 2011

Absolutely, can I ask which version of node you are using? When I wrote the code I was using version 0.2.5 which would be considered quite old now-a-days :-)

@bitter
Copy link
Author

bitter commented Mar 7, 2011

Hmm... I just realized that running the tests using node is not such a good idea since the only thing they prove is that the tests can be run using a requirejs adapter. I would like to rewrite the test to be a browser test instead, that way the htmlparser (and the tests) will be loaded using requirejs.

@bitter
Copy link
Author

bitter commented Mar 7, 2011

It looks like a lot has changed since I created my patch and I'm not sure the patch is valid anymore. I'll dig some more but as of now I would recommend that you drop the RequireJS compatibility, I'll try to make a new patch when I figure out the state of the RequireJS node adapter.

@tautologistics
Copy link
Owner

I have been working against node v0.4.1 and started with your version of r.js. Then that failed I pulled the latest from requirejs.org and still got the same thing. Unless there is a big problem, there can still be a requirejs version of the runtests.js in addition to the runtests.html; the more coverage the better.

For now the changes will remain in the bitter-master branch until you have an update or I have some time.

Conflicts:
	lib/node-htmlparser.js
siddMahen referenced this pull request in siddMahen/node-htmlparser Jan 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants