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

Offer FS-free API #21

Closed
shakyShane opened this Issue Jun 11, 2014 · 11 comments

Comments

2 participants
@shakyShane

shakyShane commented Jun 11, 2014

First of all, a huge thanks!

I have been using your SVG tools for over a year now on many projects (through both the Grunt Plugin, & your web app) & they are flawless!

I have my own own project for creating SVG sprites for gulp - but it's full of rendering problems - to the point where I don't even use it my own projects (I use Iconizr as stated above.)

So, I wanted to 'borrow' all of the awesome SVG work you've been doing & wrap it in my streaming interface for gulp... but currently your lib does not allow this.

Basically, it's a shame to have FS access littered throughout your lib as it limits the use-cases dramatically.

Your lib is solving SVG related problems in a great way & it would be awesome to make the core consumable by other libs.

As a proof-of-concept, I forked & modded this: https://github.com/shakyShane/svg-sprite/blob/master/lib/svg-sprite.js

And you can see how you'd consume it here https://github.com/shakyShane/svg-sprite/blob/master/example-stream.js (this example being a stream).

It simply adds the addFile method which takes content as a string. This approach limits the code in the lib to doing what it does best - SVG manipulation. Implementing an interface like this would allow this project to be the core of other tools and basically be even more awesome!

I can see two options if you like the sound of this - moving the SVG stuff into a separate lib that does NO file access, or simply adding additional methods to this lib which can skip the FS stuff.

Personally, I think a separate lib would be amazing and would allow easier maintenance & iterations without the need for all the FS boilerplate. If it just works on strings & returns strings, it becomes infinitely more useful.

Thanks for listening, interested in your thoughts (I really don't want to publish a 'fork' as my own module). :)

@shakyShane

This comment has been minimized.

shakyShane commented Jun 11, 2014

also, forgot to mention that I'd be happy to help/contribute :)

@jkphl

This comment has been minimized.

Owner

jkphl commented Jun 11, 2014

Hi Shane,

thanks a lot for your kind words and all the input! In fact, I've already been thinking about creating a Gulp port of svg-sprite and iconizr (e.g. see here). Basically there are two things that keep me from doing so at the moment, which are

  • my very limited time resources and
  • my very limited knowledge of Gulp.

However, what you're saying sounds very reasonable and I'm pretty up for moving into that direction! Please give me some time to look into your fork, and then let's see how we could manage this. I'd definitely very much appreciate your advise and contribution!

I'll get back to you as soon as possible! :)

Cheers,
Joschi

@jkphl

This comment has been minimized.

Owner

jkphl commented Jun 11, 2014

@shakyShane Oh, and what I forgot to mention: In fact, the svg-sprite and node-iconizr modules have always been intended to be as universal as possible (as opposed to the grunt-svg-sprite and grunt-iconizr derivates). Of course, due to my limited knowledge of Gulp etc. (and keeping in mind that they are my first and only Node modules so far), they could be much more independent as they are at the moment. Having said that, your suggestions are more than welcome! ;)

@shakyShane

This comment has been minimized.

shakyShane commented Jun 11, 2014

Great to hear that you're open to suggestions.

Just to clarify my point though, it's not really about creating a gulp plugin. It's about extracting the functionality so that it can accept strings & return the data. It would then be up to the implementing library to handle the FS stuff.

The side-effect of doing it that way is that building something like a gulp plugin would be trivial (that's why I created that example).

You simply couldn't write a Gulp plugin for your lib in it's current state as Gulp relies on API's that don't access the FS.

@shakyShane

This comment has been minimized.

shakyShane commented Jun 11, 2014

so you could still offer the API you do now (accepting file paths), but behind the scenes it would use the extracted SVG functionality.

Then anyone else can come along & use the SVG functionality without all of the FS stuff and do whatever they want with the output.

@jkphl

This comment has been minimized.

Owner

jkphl commented Jun 11, 2014

Yes, I totally understand your point. I'll think about it. I believe there must remain a simple solution for people that "just want to make sprites" (e.g. on the command line) without using additional modules / libraries. So intuitively, I'd probably tend to restructure the svg-sprite library as you described (keeping the current API, but changing the internals).

@shakyShane

This comment has been minimized.

shakyShane commented Jun 11, 2014

^ That would be awesome!

jkphl added a commit that referenced this issue Dec 29, 2014

@jkphl

This comment has been minimized.

Owner

jkphl commented Dec 29, 2014

Hi @shakyShane,

it took me quite a while, but I just published the next major release of svg-sprite. It's rewritten from scratch and — as you suggested — I stripped off all file-system access. Both a Grunt and a Gulp plugin are on their way ... ;)

Btw congrats to gulp-svg-sprites, awesome work! :)

Cheers & a happy new year,
Joschi

@jkphl jkphl closed this Dec 29, 2014

@shakyShane

This comment has been minimized.

shakyShane commented Dec 29, 2014

Awesome work!

Ping me when the gulp plugin is ready - I'd like to deprecate gulp-svg-sprites & direct people to yours instead :)

@jkphl

This comment has been minimized.

Owner

jkphl commented Dec 29, 2014

Oh, wow, sure! In fact, the gulp plugin is already finished and working, just need to add and perform some more test. I'll let you know (in 2014 still, I guess). Cheers!

@jkphl

This comment has been minimized.

Owner

jkphl commented Dec 30, 2014

Hey Shane, it's finally done! I'd very much appreciate your feedback — but please be forgiving, that's my first Gulp thingie ever ... ;)

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