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

Remove unnecessary step with number separator from Color function #208

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

slaweet
Copy link
Contributor

@slaweet slaweet commented Aug 2, 2021

@Qix- I'm sorry that my comment https://github.com/Qix-/color/issues/204#issuecomment-889933878 came across as demanding and thankless. I'll try to be more constructive and look for a solution that is aligned with your aims of this project. What do you think about the change in this PR? Unlike transpiling, it should AFAIK not add any maintenance cost.

AFAIK object &= 0xFF_FF_FF; was an unnecessary step, because

object & 0xFF is equivalent to object & 0xFF_FF_FF & 0xFF and
(object >> 16) & 0xFF is equivalent to ((object & 0xFF_FF_FF) >> 16) & 0xFF

This should prevent the error described in https://github.com/Qix-/color/issues/204

There is just one more place that would throw a similar error:

color/index.js

Line 257 in 694d197

lum[i] = (chan <= 0.039_28) ? chan / 12.92 : ((chan + 0.055) / 1.055) ** 2.4;
I want to try to find a mutually acceptable solution for that one as well, but first I want to know if this solution is acceptable, or there is still something more I didn't consider?

Note that I don't have anything against ESM. I have read through https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c that you recommended and I didn't find there anything about ECMAScript 2021 features (such as Numeric separators). I'm using ESM in my code that is built/compiled with https://cli.vuejs.org/. It also works well with ESM in NPM dependencies that use the package.json module field. The only thing that is causing issues is ES2021 numeric separators.

AFAIK `object &= 0xFF_FF_FF;` was an unnecessary step, because

`object & 0xFF` is equivalent to `object & 0xFF_FF_FF & 0xFF` and
`(object >> 16) & 0xFF` is equivalent to `((object & 0xFF_FF_FF) >> 16) & 0xFF`

This should prevent the error described in https://github.com/Qix-/color/issues/204
@Qix-
Copy link
Owner

Qix- commented Aug 4, 2021

I'm using ESM in my code that is built/compiled with https://cli.vuejs.org/

Then an issue needs to be filed with the Vue team as they do not support modern Javascript. I'll accept this PR as it is definitely a no-op but not because it solves a syntactic problem.

I linked to that as it is directly the spirit of modernizing code. That's all.

The library is standard javascript, it's just too new for your specific project. That stance I will not budge on, sorry.

@Qix- Qix- merged commit 4647eae into Qix-:master Aug 4, 2021
@Qix-
Copy link
Owner

Qix- commented Aug 4, 2021

Published as 4.0.1.

@slaweet slaweet deleted the remove-unnecessary-step branch August 5, 2021 17:29
@slaweet
Copy link
Contributor Author

slaweet commented Aug 5, 2021

@Qix- thank you for accepting the PR

Then an issue needs to be filed with the Vue team as they do not support modern Javascript.

Just to share a bit more context. With Vue cli default config, I can use numeric separators in my own code and it will transpile it using https://webpack.js.org/loaders/babel-loader/ under the hood, so I think that they do support modern Javascript. But everything inside node_modules is by default excluded from transpilation (for performance reasons).

@Qix-
Copy link
Owner

Qix- commented Aug 5, 2021

Yes but if Vue's parser doesn't support it then it's missing an accepted and standardized feature of Javascript. They should be able to implement it quickly and push out a fix that doesn't require babel. My package isn't the place to fix that; I've done my due diligence with the major release cycle.

The alternative is to ask the XO.js team to remove that rule but I don't see them doing that.

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

Successfully merging this pull request may close these issues.

2 participants