harrisiirak / cron-parser

Node.js library for parsing crontab instructions
MIT License
1.33k stars 156 forks source link

Why not splitting the parser from the expression? #112

Open yanickrochon opened 7 years ago

yanickrochon commented 7 years ago

I understand that some may need this, but given the triviality of the implementation, and how useless it is in the browser, why does this method actually exist in this package? Of course, who cares if it adds a few bytes when exported for the browser, but wouldn't a peer dependency be best suited for this method, as it is not a general use case?

Just wondering.

yanickrochon commented 7 years ago

Actually, why not creating two modules from this one? I actually only need the expression parser, and I got a little confused at the purpose of this module when looking at the README. If I am not mistaken, this module is about parsing crontab files / strings, while including the actual cron expression parser.

If both crontab parser and expression parser would be separate modules, my first comment would simply be invalidated.

If they were distinct modules, it would also be easier to maintain, IMO.

harrisiirak commented 7 years ago

Hi @yanickrochon! Thanks for your feedback.

I see your point keeping this module efficient and functional as possible. Although it's not adding much logic/weight to the module, it still needs attention when something changes or will be added. Definitely this is not happening in 2.x because it may break backward compatibility for some of the users. This something that can be done in 3.0.

My mind is open for this idea.

yanickrochon commented 7 years ago

(Thank you for correcting the auto-correct :stuck_out_tongue: )

Yes, I this is why I was linking to the other issue, and implementing the API for ES6 syntax, targeting Node 7 and upward. This would be for a next major release, as it would create significant enough API change to break some user code.

I would propose splitting this way :