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 Browserify / CommonJS support #170

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
@wookiehangover
Copy link
Contributor

wookiehangover commented Jan 13, 2014

This makes it possible to require Snap.svg with Browserify and modifies package.json to point towards the correct module entry point instead of the Gruntfile.

Supporting AMD is great and all, but it's only a few more lines to support CommonJS style module systems when you're already using the factory pattern. Plus, npm is already being used to install eve, so I didn't even have to modify dependencies to get things working. Edit: looks like eve was only included as a devDependency, so that needed to be changed, too.

Thanks in advance!

@@ -50,8 +50,8 @@ Snap.toString = function () {
};
Snap._ = {};
var glob = {

This comment has been minimized.

@wookiehangover

wookiehangover Jan 13, 2014

Author Contributor

FYI I found the conflicting glob objects (the one passed to the factory that references window and the one being created here) to be a bit of a head-scratcher.

Is it really worth the potential misdirection to save a handful of characters? Wouldn't a module scope win and doc be just as useful without needing to shadow the other glob?

@splodingsocks

This comment has been minimized.

Copy link

splodingsocks commented Jan 18, 2014

@wookiehangover thanks for doing this. I'm trying to use this right now. I've installed it as a bower package, but requiring it with debowerify still throws an error: Cannot read property 'contains' of undefined from line 2024 of the unimified version. Is it bad to try to use this as a Bower library now that you've packaged it for npm?

@dotob

This comment has been minimized.

Copy link

dotob commented Jan 25, 2014

this looks promising, hope it will be merged soon.

@jazzzz

This comment has been minimized.

Copy link

jazzzz commented Apr 11, 2014

👍

@murphyrandle you have to update the dist or use the wookiehangover/browserify-dist branch

@frhd

This comment has been minimized.

Copy link

frhd commented May 23, 2014

Can someone point me to how to use this fix in current snapsvg v0.2.1?

@VinSpee

This comment has been minimized.

Copy link

VinSpee commented May 29, 2014

Is this going to happen? I'd love for it to happen.

@DmitryBaranovskiy

This comment has been minimized.

Copy link
Contributor

DmitryBaranovskiy commented May 29, 2014

Could you change this pull request to target dev branch instead of master and sign CLA? Then I will merge it today.

@lmartins

This comment has been minimized.

Copy link

lmartins commented Jul 5, 2014

Not of +1 comments myself but seeing this PR at this state makes me want to comment that I would also love to have CommonJS support by SnapSVG

@Gaffen

This comment has been minimized.

Copy link

Gaffen commented Sep 5, 2014

+1

@Klowner

This comment has been minimized.

Copy link

Klowner commented Sep 16, 2014

👍 this would improve my life.

Sam Breed
Merge remote-tracking branch 'upstream/dev' into browserify-support
* upstream/dev:
  Fix for pattern methods
  Fix regression from v0.2.0 when animating transforms
  Added toDataURL() methods to paper and element. Extracted element methods as module.
  Fixed #196
  Fixed issue with parse in IE
  Fix for Paper.use and new method Paper.symbol
  Fixed bug with clipPath having id and paper.use
  Fixed #280
  Moved class methods in to separate module and added docs for them
  Typo in dr.js comments
@wookiehangover

This comment has been minimized.

Copy link
Contributor Author

wookiehangover commented Sep 16, 2014

@Klowner looks like we both had the same idea this morning :)

@DmitryBaranovskiy where's the CLA for me to sign?

@Klowner

This comment has been minimized.

Copy link

Klowner commented Sep 16, 2014

@wookiehangover

This comment has been minimized.

Copy link
Contributor Author

wookiehangover commented Sep 16, 2014

done & done.

@DmitryBaranovskiy ready to merge 👯

@DmitryBaranovskiy

This comment has been minimized.

Copy link
Contributor

DmitryBaranovskiy commented Sep 17, 2014

Make this pull to dev instead of master and will be happy to merge it. :)

@Klowner

This comment has been minimized.

Copy link

Klowner commented Sep 17, 2014

#300 is ready to go against dev branch if it's cool with @wookiehangover

@zetta83

This comment has been minimized.

Copy link

zetta83 commented on 28b7a7a Feb 3, 2015

This fix dont work in IE11, becose after 'div.innerHTML = E;' f.childNodes is empty.
f = svg; - creatre link to div content, need f = svg.cloneNode(true);

@n370

This comment has been minimized.

Copy link

n370 commented Feb 3, 2016

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment