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

Implement missing transformers #1

Closed
74 of 89 tasks
ForbesLindesay opened this issue Feb 6, 2015 · 113 comments
Closed
74 of 89 tasks

Implement missing transformers #1

ForbesLindesay opened this issue Feb 6, 2015 · 113 comments
Labels

Comments

@ForbesLindesay
Copy link
Member

This project aims to replace transformers with a large collection of separate libraries, each of which able to be separately maintained, tested and updated. Each library will implement some subset of the API in this module, and may optionally return a string body rather than the {body: String, dependencies: Array.<String>} format (lots of file formats don't actually have dependencies so it wouldn't really make sense). This module can then be used to fully normalise that interface, including implementing the methods that are otherwise not implemented.

Here is a list of all the modules that need implementing

Potential Extras

Once all of these have been implemented, we can switch jade to look for these rather than transformers. I'll need as many collaborators as possible for this, so if you're willing to contribute a transformer, let me know and I'll add you to the organisation. P.S. note that we add jstransformer to the keywords in package.json for all transformers, so they can be found at https://www.npmjs.com/browse/keyword/jstransformer

@ForbesLindesay ForbesLindesay changed the title Implement missint transformers Implement missing transformers Feb 7, 2015
@ForbesLindesay
Copy link
Member Author

@bloodyowl @enlore @hemanth @jbnicolai @jeromew @rlidwka @TimothyGu would any of you like to help out?

@hemanth
Copy link
Member

hemanth commented Feb 9, 2015

Count me in :)

@ForbesLindesay
Copy link
Member Author

Awesome, welcome aboard :)

Please make sure you add forbeslindesay as an owner if you publish any new jstransformers to npm. I'll write a bot at some point to add all of the owners on GitHub as owners on npm :)

@jbnicolai
Copy link

I'm game as well ^^

I'll write a bot at some point to add all of the owners on GitHub as owners on npm

Good call.

@hemanth
Copy link
Member

hemanth commented Feb 9, 2015

@ForbesLindesay Here we go! Started with jstransformer-handlebars (Made it look as close as your code.)

@ForbesLindesay
Copy link
Member Author

Looks good. Just had a thought though:

For templating engines we should support two step compilation (i.e. compile then render). I would suggest we support:

exports.compile = function (str, options) {
  reutrn function (locals) { /* return resulting string */ };
};
// or
exports.compile = function (str, options) {
  reutrn {fn: function (locals) { /* return resulting string */ }, dependencies: [/* array of files */]};
};

We could the have template.render behave like the following by default:

exports.render = function (str, options, locals) {
  var compiled = exports.compile(str, options);
  if (typeof compiled === 'function') return compiled(locals || options);
  else return {body: compiled.fn(locals || options), dependencies: compiled.dependencies};
};

I'll do underscore and then update this library to show what I mean.

@hemanth
Copy link
Member

hemanth commented Feb 9, 2015

Yup, compile and render makes sense and they are convenient as well.

@TimothyGu
Copy link
Member

@ForbesLindesay sure I can use some fun porting modules over.

@bloodyowl
Copy link

I'll try to find the time to do some of them

@TimothyGu
Copy link
Member

@ForbesLindesay how about caching? Do we let the libraries handle caching or do we use a DIY one like in transformers?

@TimothyGu
Copy link
Member

Jade transformer written. Function signatures look like this:

compile(source, options);
compileClient(source, options);
render(source, options, locals);

If this is OK I'll go ahead and write the transformer for EJS too.

@ForbesLindesay
Copy link
Member Author

Looking at what's actually in that repo. It looks perfect. i.e.

compileFile(source, options);
compileFileClient(source, options);

@ForbesLindesay
Copy link
Member Author

For caching: wherever possible we disable all caching. The dependencies array is there so that it is easy for an application to tell when a file needs to be rebuilt. If the user wants caching, they should use the Transformer.compile interfaces instead of the Transformer.render interfaces and then they can cache the result of that.

The only part that I'm not 100% sure on is whether we should do anything for dynamic caches like browserify supports where you avoid re-parsing all the files that haven't changed. Currently my intention is not to do anything special for those unless someone asks for it.

@TimothyGu
Copy link
Member

@ForbesLindesay sounds good.

@JosefJezek
Copy link

Please ✨ markdown-it ✨ - modern pluggable markdown parser

See https://www.npmjs.com/browse/keyword/markdown-it

@TimothyGu
Copy link
Member

@JosefJezek TODO added.

Uglify-JS and JSON beautifier/uglifier ported.

@TimothyGu
Copy link
Member

verbatim and cdata* are ported.

@hemanth
Copy link
Member

hemanth commented Feb 16, 2015

@ForbesLindesay I have implemented few of them, but haven't published it yet, as you had mentioned earlier that add all of the owners on GitHub as owners on npm, should I go ahead a publish them or wait?

@TimothyGu
Copy link
Member

@hemanth I already published the ones ported by me. We can change the owners after we publish anyway.

@TimothyGu
Copy link
Member

@hemanth regarding @ForbesLindesay's suggestion of writing a bot that fetches GitHub owners and automatically add them to npm, it's technically not possible as npm login and GitHub login might be different (e.g. ForbesLindesay (GitHub) vs forbeslindesay (npm)). What we can do is add all of the owners to one repo (i.e. this one) and do something like this:

owners="`npm owner ls jstransformer | cut -d' ' -f1`"

for p in `curl -s 'https://api.github.com/orgs/jstransformers/repos' | sed -n 's/.*"name".*:.*"\(.*\)",/\1/pg'`; do
    for u in $owners; do
        npm owner add "$u" "$p"
    done
done

(Bash FTW)

@hemanth
Copy link
Member

hemanth commented Feb 16, 2015

@TimothyGu Nice, we must also update the package.json with authors or have a common author say like jstransformer.

@TimothyGu
Copy link
Member

@hemanth I would argue against that, as 1. package.json cannot have more than one authors, only maintainers, and 2. not everybody will necessarily be maintaining every module, and adding the person to maintainers would just be dubious.

@ForbesLindesay
Copy link
Member Author

@TimothyGu I was thinking that we would maintain a file somewhere that listed all project owner's npm accounts.

@hemanth go ahead and publish them, but if possible please add me as an owner on npm via:

npm owner add forbeslindesay

@hemanth
Copy link
Member

hemanth commented Feb 16, 2015

@ForbesLindesay Done.

@TimothyGu
Copy link
Member

@ForbesLindesay that's fine too, but the question is where.

@RobLoach
Copy link
Member

markdown-ithttps://github.com/RobLoach/jstransformer-markdown-it

I requested a transfer, but don't have access.

@TimothyGu
Copy link
Member

@RobLoach done. I've also invited you to our maintainer team, so you can now push to that repo.

@RobLoach
Copy link
Member

Thoughts on jstransformer-es6-templates? It's a bit hacky, so I'm not sure if it's something for /jstransformers.

@tunnckoCore
Copy link
Member

👍 I think for it before few days, but using https://github.com/medikoo/es6-template-strings instead.

@tunnckoCore
Copy link
Member

@RobLoach seems to be better to use medikoo's package, its more up-to-date.

@RobLoach
Copy link
Member

I think for it before few days, but using https://github.com/medikoo/es6-template-strings instead.

Thanks! Good find.... Updated and moved to here:
https://github.com/jstransformers/jstransformer-es6-template-strings

@tunnckoCore
Copy link
Member

@RobLoach see the commit comment :)

@RobLoach
Copy link
Member

@stoeffel Used generator-jstransformer for jstransformer-es6-template-strings, jstransformer-whiskers and jstransformer-styl. It worked perfectly 👍 .

@tunnckoCore
Copy link
Member

@RobLoach yep, but yeoman is too big and too shitty, same as gulp. lol

btw, should run add-owners for ur packages :)

@tunnckoCore
Copy link
Member

💯 jstransformer-absurd 💯 added to the list and almost finished, it is little tricky because not so node-friendly api.

@stoeffel
Copy link
Contributor

@RobLoach nice 😎

@tunnckoCore
Copy link
Member

hm.. both coffeekup and coffeecup looks very outdated and are similar, so i think we should implement only one of them?

@tunnckoCore
Copy link
Member

coffeekup - done

@tunnckoCore
Copy link
Member

list is sorted again
added to the list

  • autoprefixer
  • cssnext
  • doiuse
  • fest
  • gaikan
  • rtlcss
  • slm-lang
  • resin

@ForbesLindesay
Copy link
Member Author

I'm not sure about the explosion of postcss based transformers. I feel like maybe we should be taking the approach of the less transformer and accepting a plugins option instead of having a jstransformer per plugin. One very real downside of the approach we are taking at the moment is that a pipeline of postcss transformations could have very poor performance.

@tunnckoCore
Copy link
Member

a plugins option

Yea, we will, I'll update postcss, rework and styl later today. But they are almost separate transformation tools, commonly used and it will be just sugar of doing install jstransformer-postcss, install cssnext, install autoprefixer separately.

This dont have bad side.

is that a pipeline of postcss transformations could have very poor performance.

And? This isn't jstransformer(s) problem.

@RobLoach
Copy link
Member

https://github.com/jstransformers/jstransformer-browserify

Plugins

I think it should take an options.plugins parameter, and process it accordingly in preparing .compile/.render. Having to build a Transformer for each plugin is a lot of maintenance work for us, and limits the abilities of what the transformer can do.

As a side note, we'll need this plugins pattern for Browserify too. Made an issue: jstransformers/jstransformer-browserify#2

@RobLoach
Copy link
Member

@jstransformers/owners First pass on https://github.com/jstransformers/jstransformer-cli .... Let's get discussions going on over in the issue queue there.

@tunnckoCore
Copy link
Member

Having to build a Transformer for each plugin is a lot of maintenance work for us, and limits the abilities of what the transformer can do.

i'll repeat again, there's no bad side of this. it not limit what jstransformer can/will do.
and no, no "more work for us" when the pkg have quality and good tests, plus I'm very active every single day.

@ForbesLindesay
Copy link
Member Author

@tunnckoCore The thing is that maintaining those is out of scope for jstransformers. It is crucial to the success of any organisation that it remain focused. There are three reasons this is really important:

  1. There is another, much better way for us to support this, via a simple "plugins" option. This is idiomatic of how jstransformers are generally expected to work (see jstransformer-less for example).
  2. By forcing each plugin to be a separate jstransformer, we make some functionality impossible. For plugins that take x -> y rather than x -> x you can't always break things up in the way you want to. Consider, for example, browserify transforms. You have to give all the transforms you want to the same, single, browserify instance. You simply cannot do it in multiple passes.
  3. Doing multiple passes can create a massive performance bottleneck. Say I want to use cssnext and doiuse from postcss: if I use the plugins system, it parses the css once, applies two transformations to the AST, then outputs that AST to a string. If I use your approach, I have to parse the AST and then stringify it for each plugin I want to make use of. That performance bottleneck is our problem, not the problem of postcss.

@tunnckoCore
Copy link
Member

@ForbesLindesay
It seems you dont understand me correct. Im not against plugins option and i'll implement it in postcss. The idea is to support separate jstransformers only for some of postcss plugins which walk their way to the future as separate tools. It would be more clean and lean to use just jstransformer-cssnext, not using plugins option for only one plugin.

Choosing to use separate jstransformers in this way, when we have support for both ways would be idiotism, yea. And this would be user's implementation/architecture design mistake, not our problem - we all have heads on our arms to think. I just won't support users that cant think, that dont have some principles and at least little basic knowledge about "what and how", that dont have logical/rational mind.

Regards, Charlike. :)

@tunnckoCore
Copy link
Member

Its looks better

var transformer = cssnext.renderFile('path/to/foo/bar.css', {})

// instead of
var transformer = postcss.renderFile('path/to/foo/bar.css', {plugins: [cssnext()]})

@tunnckoCore
Copy link
Member

resin done, but i didn't notice that the resin package is out-of-date and they move to topcoat-resin and republish, soo yea shitty shits.

postcss and rework now support options.plugins.

@RobLoach
Copy link
Member

RobLoach commented Jun 2, 2015

@RobLoach
Copy link
Member

@tunnckoCore
Copy link
Member

@RobLoach haha, you're good follower! 🐈

@hemanth
Copy link
Member

hemanth commented Jun 26, 2015

We still have few left in the potential extras list.

@RobLoach
Copy link
Member

RobLoach commented Jul 2, 2015

I've created a Label for this, and added each Transformer as an issue so that we can track them individually:
https://github.com/jstransformers/jstransformer/issues?q=is%3Aopen+is%3Aissue+label%3ATransformer

Considering all of our projected initial requirements are met, I'll close this now that we have the other issues open. Great work, guys!

@RobLoach RobLoach closed this as completed Jul 2, 2015
@ForbesLindesay
Copy link
Member Author

👍 awesome work.

Thanks so much for supporting this project everyone. I'm so excited for how big this is becoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants