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

[incremental-dom] - Add incremental dom #361

Merged
merged 3 commits into from Jan 13, 2016
Merged

Conversation

tommichaelis
Copy link
Contributor

This adds incremental dom to cljsjs.

Incremental dom is google closure compiler compatible, so this package plays nicely with advanced compilation. However, the default incremental dom build uses goog.module to declare the namespace, which doesn't seem to be compatible with the clojurescript compiler's dependency resolution. I've included a patch for the incremental dom gulp file which outputs code which is compatible with the clojurescript compiler.

I'm not sure what the build server supports, but this will require nodejs and npm to build the library. If that's not supported, I'd welcome any suggestions as to the best way to get this into cljsjs.

@Deraen
Copy link
Member

Deraen commented Jan 12, 2016

Hmm, interesting. CI should support Node and npm.

I would like to check if we can get ClojureScript to support goog.module definitions, with a quick glance doesn't look like too hard. Could you provide me a version without the patch for me to test, for example in another PR?

I think without patch there is no need to use Make.

@Deraen
Copy link
Member

Deraen commented Jan 12, 2016

Adding Cljs support for goog.module is tracked here: http://dev.clojure.org/jira/browse/CLJS-1543

@tommichaelis
Copy link
Contributor Author

I've added a pull request which uses goog.module (#362).

As an alternative, the original library is written using ES2015 modules, and also has gulp tasks to compile to CommonJS and AMD modules. I don't think any of those module systems are supported in the clojurescript compiler either, but if there's more impetus behind adding any of these it might be worth trying to integrate one of those options.

@tommichaelis
Copy link
Contributor Author

I've just updated this to use just boot. It still uses the patched gulpfile, but I did enough reading around the boot api to get it to work without shelling out to make.

@Deraen
Copy link
Member

Deraen commented Jan 13, 2016

Great! As adding support for goog.module in Cljs compiler wil take some time, lets merge this.

Could you just fix the header in readme :)

@tommichaelis
Copy link
Contributor Author

Fixed - cheers :)

@Deraen Deraen merged commit b2a7ab6 into cljsjs:master Jan 13, 2016
@Deraen
Copy link
Member

Deraen commented Jan 13, 2016

I did just once change, I added build identifier to version: -0.

Should be deployed now, there was some errors from gulp related to unit-phantom, but I don't know if that matters? Probably CI has older PhantomJS version or something like that.

@tommichaelis
Copy link
Contributor Author

Sounds good, just tested it and it all seems to work as expected!

Cheers Deraen

@Deraen
Copy link
Member

Deraen commented Jan 13, 2016

Thanks for contributing the package!

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.

None yet

2 participants