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 simple shim so that importing metal-soy does not throw error when importing in Node.js environment #265

Closed
wants to merge 1 commit into from

Conversation

robframpton
Copy link

@robframpton robframpton commented Oct 2, 2017

See #264

@jbalsas
Copy link
Contributor

jbalsas commented Oct 10, 2017

Hey @Robert-Frampton, for this... we are pretty much working around https://github.com/mairatma/html2incdom, specially because of how it includes HTMLParser, iirc.

I'd like to keep html2incdom under control, moving it to the metal org. With that in mind... does it make sense to keep the existing one? Could we do it in a way that it uses modules properly and doesn't rely on globals? Mabye we could even use a different HTMLParser that we didn't need to maintain...

What do you think?

@robframpton robframpton changed the base branch from master to develop October 10, 2017 16:24
@robframpton
Copy link
Author

Hey @jbalsas

I agree that we should be maintaining that functionality. What do you think about adding this API to metal-incremental-dom rather than creating a new package?

@jbalsas
Copy link
Contributor

jbalsas commented Oct 10, 2017

That sounds lovely, @Robert-Frampton! 😍

@robframpton
Copy link
Author

I've sent #274 which copies html2incdom into metal-incremental-dom. It also slightly modifies the thirdparty html parser so that it just exports the parser rather than attaching it to the window.

We might be able to replace it altogether with another parser, but I think we should go with this for now to avoid regressions.

@jbalsas
Copy link
Contributor

jbalsas commented Oct 11, 2017

I assume #274 superseeds this one, right? Please reopen if we still need to merge this.

@jbalsas jbalsas closed this Oct 11, 2017
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.

2 participants