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

Add dom.js to allow custom document object #165

Merged
merged 6 commits into from
Oct 25, 2019

Conversation

saschanaz
Copy link
Contributor

Closes #158

👍

tests/index.js Outdated Show resolved Hide resolved
@goto-bus-stop
Copy link
Member

Thanksss! this basically LGTM if the test failures can be fixed. lmk if you need a hand there.

@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 24, 2019

@goto-bus-stop Currently test:transform-browser fails regardless of the node version, do you have any idea? 🤔

Edit: I think it's some kind of browserify problem.

@saschanaz
Copy link
Contributor Author

Removed .transform(nanohtml) because 1) per the browserify document it the argument should be a function that returns a file stream but nanohtml is not such thing 🤔 2) browserify(require.resolve('../browser')) should already include nanohtml anyway?

@goto-bus-stop
Copy link
Member

the node.js version of nanohtml triples as a browserify transform and a babel plugin!

the test:transform-browser makes sure that using the browserify transform produces the same output as not using it, so it's important that it stays.

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Oct 24, 2019

(This is what i mean lol:

nanohtml/lib/server.js

Lines 11 to 18 in 6d0dc18

function nanothtmlServer (src, filename, options, done) {
if (typeof src === 'string' && !/\n/.test(src) && filename && filename._flags) {
var args = Array.prototype.slice.apply(arguments)
return require('./browserify-transform.js').apply(this, args)
}
if (typeof src === 'object' && src && src.types && src.template) {
return require('./babel').apply(this, arguments)
}
)

So the problem with the browserify transform test is that the transform looks for a var html = require('nanohtml') in the source files. The aliasify stuff in the transform test rewrites require('../../') to require('nanohtml') so that the nanohtml transform picks up on it. This PR changes it to var { html } = require('./html'), which nanohtml does not recognise, and can't easily be rewritten to something that nanohtml recognises.

i know you don't like mutating the global, but i think the easiest way to address this would be to create the JSDOM object in test/index.js and assign its window and document to the global object before doing require('./browser'), and keeping the browser test files intact.

e; sorry this is spiraling into something much more complex than anticipated!

@saschanaz
Copy link
Contributor Author

i know you don't like mutating the global, but i think the easiest way to address this would be to create the JSDOM object in test/index.js and assign its window and document to the global object before doing require('./browser'), and keeping the browser test files intact.

Understood, but doing so won't really test the new dom.js, no? Wouldn't it just run as if it's on a browser with global document object?

@saschanaz
Copy link
Contributor Author

Hmm, browser.js now internally uses dom.js so it should be same. Okay, will do it.

@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 24, 2019

Went a bit different way as your suggestion somehow didn't work (because of conflicts between browser and server tests). It now works 😁

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

Thanks!!

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

Thanks!!

@saschanaz
Copy link
Contributor Author

Oops, I think the aliasing is now redundant 👀

@goto-bus-stop
Copy link
Member

The tests were ok either way, but aliasing both paths ensures that the main nanohtml code doesn't end up in the bundle, which makes the tests a little stronger. We should also add checks to the transform and babel plugin tests to make sure that we are not accidentally including the entire nanohtml module anyway because that would defeat the purpose :D

@goto-bus-stop goto-bus-stop merged commit bfb3aef into choojs:master Oct 25, 2019
@goto-bus-stop
Copy link
Member

📦 1.9.0 🎉

@saschanaz saschanaz deleted the dom branch October 25, 2019 10:10
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.

Custom DOM library support on Node.js (e.g. JSDOM)
2 participants