mapbox / carto

fast CSS-like map stylesheets
https://cartocss.readthedocs.io/
Apache License 2.0
652 stars 129 forks source link

adds support for browserify #379

Closed javisantana closed 9 years ago

javisantana commented 9 years ago

adds support for browserify, the final javascript file can be generated using make target:

make dist/carto.uncompressed.js

It exposes the global variable "carto" so can be used in the browser like:

carto.Parser().parse('#test { marker-fill: #F00; }')

One of the problems is mapnik reference, the module is not read to work with browserify since it loads modules dynamically. It does not make sense to make it compatible since mapnik-reference is not going to be used so people must to set the reference needed before parse/render:

carto.tree.Reference.setData(reference)

this PR also changes some code to make compatible with the browser.

tmcw commented 9 years ago

It exposes the global variable "carto" so can be used in the browser like:

If we're going to support browserify, we should only export as a module, and never touch globals without permission.

One of the problems is mapnik reference, the module is not read to work with browserify since it loads modules dynamically.

I'm not comfortable papering over this problem by deferring the failure to whenever someone calls a documented API.

if (typeof(this['window']) !== undefined) {

This should use process.browser instead.

Why does this require a listing of all tree files as requires?

Basically:

If we want to support browserify, a proper module compatibility should come first, not a bundled standalone build.

javisantana commented 9 years ago

If we're going to support browserify, we should only export as a module, and never touch globals without permission.

this build is created to work in a browser, it's a common pattern to expose global scope to use it in javascript, maybe there should be a build specifically for it ?

I'm not comfortable papering over this problem by deferring the failure to whenever someone calls a documented API.

basically that is a workaround for carto being attached to mapnik-reference, what do you propose?

Why does this require a listing of all tree files as requires?

because of this dynamic loading: https://github.com/mapbox/carto/blob/master/lib/carto/index.js#L67

If we want to support browserify, a proper module compatibility should come first, not a bundled standalone build.

not sure if a follow you here, what is exactly "a proper module compatibility" ?

tmcw commented 9 years ago

because of this dynamic loading

Okay, so that shouldn't be dynamic - right now we're trading a bit of brevity for browserify compatibility, let's trade back.

this build is created to work in a browser, it's a common pattern to expose global scope to use it in javascript, maybe there should be a build specifically for it ?

Browserify is here to make

var carto = require('carto');

Work: to make CommonJS modules usable as modules in the browser.

basically that is a workaround for carto being attached to mapnik-reference, what do you propose?

Making mapnik-reference work with browserify. It's possible.

not sure if a follow you here, what is exactly "a proper module compatibility" ?

To make a standalone build, you should be able to do this:

npm install carto
browserify -r carto -s carto > carto.standalone.js

But by default you should use carto as a module: in your package.json, with a commonjs include: the default way to use modules with browserify.

javisantana commented 9 years ago

Browserify is here to make [...]

browserify is here to make standalone modules too.

Making mapnik-reference work with browserify. It's possible.

Yep, the thing here is carto should not depend on mapnik-reference

thanks for your time man

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.15%) when pulling c14756e1a04e6c51a29fa5d3ae59fae8a816abf4 on javisantana:browserify into b19ade3850a23a0ecdd9999d389eee93191246ca on mapbox:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.15%) when pulling c14756e1a04e6c51a29fa5d3ae59fae8a816abf4 on javisantana:browserify into b19ade3850a23a0ecdd9999d389eee93191246ca on mapbox:master.

tmcw commented 9 years ago

Yep, the thing here is carto should not depend on mapnik-reference

Just to be clear: I agree, and it's totally possible to disentangle the two.