hail2u / node-css-mqpacker

Pack same CSS media query rules into one using PostCSS
675 stars 60 forks source link

Syntax error index.js#L139 in old Node.js #65

Closed michaelx closed 6 years ago

michaelx commented 6 years ago

https://github.com/hail2u/node-css-mqpacker/blob/a02048b3835961b08df4c3e861cc161698a134d8/index.js#L139

Shouldn’t this just be options? Making it:

module.exports = postcss.plugin(pkg.name, options => {
  const opts = {
    sort: false,
    options
  };
hail2u commented 6 years ago

There is no syntax error here. options must be expanded by a spread operator.

michaelx commented 6 years ago

I see. Problem was that the object spread syntax requires >= Node 8.3.

codebymikey commented 5 years ago

Even though the issue is marked as wontfix, would you be interested in a PR which allows this plugin to work on older Node versions?

This appears to be the only Node 8.3 feature in the project from what I could tell, so I don't think it's reasonable to force a major node upgrade in order to get the sort option functionality to work.

My implementation will be based off babel, resulting in something along the line of this:

var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };

var opts = _extends({
    sort: false
}, options);
hail2u commented 5 years ago

There is no reason to support Node.js version older than LTS.

codebymikey commented 5 years ago

I'm currently on Node v6.x which is still under LTS until 2019.

I don't see the harm in adding support for this especially since just var opts = Object.assign{sort: false}, options)) should work fine and the current code is just syntactic sugar.

If the library requires a minimum node version, then I think it should be declared in its package.json, however I don't think this project requires >= 8.x to function, so it shouldn't have that requirement.

hail2u commented 5 years ago

v6.x is not active.

codebymikey commented 5 years ago

Again, all I'm saying is that there is no strict limit stopping your library from working on node 6.x apart from you choosing not to accept a PR because you feel no-one should be using node 6.

postcss has explicit support for node 6x, so I don't see why a postcss plugin should force an upgrade when the main library doesn't.

It's your library, so I can't force you to accept my PR. I just think libraries should be as flexible as possible as long as the changes don't compromise the original purpose of the project.

hail2u commented 5 years ago

no-one should be using node 6

Yes. Using Node.js version older than LTS is your problem (or choice). It is not a problem of this package.