joewalnes / filtrex

A simple, safe, JavaScript Filter Expression compiler for end-users
MIT License
1.05k stars 74 forks source link

Patch 1 #6

Closed alexgorbatchev closed 5 years ago

alexgorbatchev commented 10 years ago

This is a house cleaning patch, has the following:

alexgorbatchev commented 10 years ago

Updated.

alexgorbatchev commented 10 years ago

Updated specs

alexgorbatchev commented 10 years ago

was i able to address all concerns? :smiley:

joewalnes commented 10 years ago

The main thing I'm concerned about is this introduces a lot of baggage and I don't understand it all. For example, make doesn't work for me anymore and I have no idea how to even begin debugging it. Clearly you understand it all and you're much more familiar with the Node ecosystem than me, so let me ask you what you're long term involvement in this project is? Are you happy to stick around as a maintainer, support issues, perform releases, etc?

I appreciate the work, I just want to make sure it makes sense for long term viability of this project.

Thanks -Joe

alexgorbatchev commented 10 years ago

I'd love to help out with house keeping and such. I do that already on a number of projects.

In terms of make, i'm not a fan at all and I pulled that from another fork. I'm concerned that it doesn't work for you. Could you try running the following from a bash script file in the same directory as Makefile?

This is equivalent to what make does

#!/bin/bash

BIN=./node_modules/.bin

rm -f src/grammar.json
rm -f src/parser.js
rm -f filtrex.js
rm -f filtrex.min.js

node src/grammar.js > src/grammar.json
$BIN/jison -j src/grammar.json -o src/parser.js
$BIN/browserify -s compileExpression src/filtrex.js > ./filtrex.js
$BIN/uglifyjs < ./filtrex.js > ./filtrex.min.js

rm -f src/grammar.json
rm -f src/parser.js
alexgorbatchev commented 10 years ago

@joewalnes wanted to follow up on this. What do you think?

joewalnes commented 10 years ago

Sorry, I've been distracted the last few days by another project. Give me a few days...

cshaa commented 6 years ago

I think this PR is worth looking into again. We found some critical security bugs and the fixes ought to be included in the NPM package.

cshaa commented 6 years ago

@alexgorbatchev Would you please look on this? You're the owner of the NPM module, so only you can update what's there. 12,106 people download the package weekly, all of that with critical bugs.

cdauth commented 6 years ago

I would really like to see this merged and to have an up-to-date version of filtrex that can be used through NPM. Would be willing to invest some work into it if maybe @joewalness could let me know what's missing for this to get merged (apart from rebasing).

cshaa commented 6 years ago

@joewalnes I think cdauth misspelled your username and you didn't get pinged. May we have your attention, please? :)