Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

adds support for browserify #379

Closed
wants to merge 4 commits into from
Closed

Conversation

javisantana
Copy link
Contributor

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.

this pr also changes some code to make compatible with the browser. It exposes the global variable "carto"
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)
@tmcw
Copy link
Contributor

tmcw commented Dec 4, 2014

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
Copy link
Contributor Author

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
Copy link
Contributor

tmcw commented Dec 4, 2014

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
Copy link
Contributor Author

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

@javisantana javisantana closed this Dec 4, 2014
@javisantana javisantana reopened this Dec 5, 2014
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@tmcw
Copy link
Contributor

tmcw commented Aug 14, 2015

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.

@tmcw tmcw mentioned this pull request Dec 4, 2015
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants