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

Update color-rgba to 3.0.0 to fix hsl color conversion bug #7325

Merged
merged 6 commits into from
Feb 18, 2025

Conversation

Lexachoc
Copy link
Contributor

@Lexachoc Lexachoc commented Jan 4, 2025

Fix the hsl conversion bug mentioned in #2431.

But the scattergl blurry problem is still out there.

@@ -80,7 +80,7 @@
"color-alpha": "1.0.4",
"color-normalize": "1.5.0",
"color-parse": "2.0.0",
"color-rgba": "2.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.
Please also commit the changes to package-lock.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@gvwilson gvwilson added community community contribution fix fixes something broken labels Jan 7, 2025
@alexcjohnson
Copy link
Collaborator

@Lexachoc I merged master since there have been some changes particularly with testing recently, but that didn't fix the failures we're seeing on CI, would you be able to look into it? In particular it's showing messages like TypeError: rgba3 is not a function from every test that tries to make a parcoords plot, looks like there may have been a breaking change in the API between v2 and v3? Presumably that'll be an easy fix, then we can see if there's anything left (maybe the change altered the colors in some of our baseline images? That would likely be an improvement but would still need updating in the tests, I can help with that part.

@Lexachoc
Copy link
Contributor Author

Lexachoc commented Feb 17, 2025

@alexcjohnson I have checked the parcoords mock test.

We're using color-rgba v2.1.1 now which is pretty old (6 years ago).
Luckily, the color-rgba was only used in parcoords throughout the entire source code, which makes debugging easier!

In v3, color-rgba uses ES Module Import instead of CommonJS, that's why we got TypeError: rgba3 is not a function.
I guess this problem occurs when Plotly switch to webpack in Plotly v3 as well?

One solution is to replace this line:

var rgba = require('color-rgba');

with:

import rgba from 'color-rgba';

then, it works. What do you think? Should we simply change the import way to make it work?

@alexcjohnson
Copy link
Collaborator

I'd love to update from require to import but I'm nervous to do just one unless we do a bunch of extra manual testing, especially given the other bundling issues we've encountered that only show up in certain environments (like #7361 and plotly/plotly.py#5027, that we just fixed with #7367).

I wonder if instead it would work to change to var rgba = require('color-rgba').default;?

cc @marthacryan

@Lexachoc
Copy link
Contributor Author

Lexachoc commented Feb 18, 2025

Yes, it also works with

var rgba = require('color-rgba').default;

I have also tested with the change from #7367, both import ways work (tested also when custom bundled).

gl2d_parcoords.json mock
image

P.S. I tested it with the test dashboard: devtools/test_dashboard/index.html

@alexcjohnson
Copy link
Collaborator

Excellent, thanks! Let’s use the require().default form for now, until we get a chance to do more extensive testing. There are a lot of ways people include this library in their projects - lots of build tools and lots of environments - so it’s not a small task to convince ourselves that a change in how files link is OK.

in `color-rgba` v3, it uses ES module.
@Lexachoc
Copy link
Contributor Author

@alexcjohnson Cheers! all tests passed.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 Excellent, thanks!

Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @Lexachoc 🚀

@emilykl emilykl merged commit 827cf94 into plotly:master Feb 18, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution fix fixes something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants