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

Wrong object returned when importing from chroma-js v2.6.0 #346

Open
aaronreed708 opened this issue Aug 6, 2024 · 17 comments · Fixed by #348
Open

Wrong object returned when importing from chroma-js v2.6.0 #346

aaronreed708 opened this issue Aug 6, 2024 · 17 comments · Fixed by #348
Labels

Comments

@aaronreed708
Copy link

We had been using chroma-js v2.4.2 in Typescript (with @types/chroma-js v2.4.4) for over 18 months in our React app.

We are using:

import * as chroma from "chroma-js"

About 6 days ago started experiencing an issue where I was getting an exception where chroma.scale was undefined. When I looked at the contents of chroma, it is an object that looks like:

{ 0: 's', 1: 't', 2: 'a', ... , default: 'static/media/chroma.271634d07525ddfb33b4.cjs }. So not like a typical chroma object. I noticed that when I build now, I end up with that chroma.xxx.cjs file under my build/static/media directory. Which is also new. When I build with v2.4.2, I don't have this file appearing in my build directory at all.

Is this just an issue because @types/chroma-js not having been updated, yet?

@gka
Copy link
Owner

gka commented Aug 8, 2024

did you update to chroma 2.5.0 or later by any chance?

@aaronreed708
Copy link
Author

Sorry @gka , looking back at my initial issue text, I wasn't very clear. This issue occurred when my build automatically pulled the latest chroma-js (v2.6.0). So this issue happens for me on both v2.5.0 and v2.6.0 and DOESN'T happen on v2.4.2.

@gka
Copy link
Owner

gka commented Aug 9, 2024

ok, that makes sense, because we refactored chroma.js to ES6 modules and changed the way, the code is imported, which is probably why the @types/chroma-js packages is no longer compatible. I'll take a look into this.

@aarthificial
Copy link

@gka Does this package follow semantic versioning or should we expect breaking changes in minor versions in the future?

@gka
Copy link
Owner

gka commented Aug 12, 2024

Apologies, I didn't expect this to be a breaking change, since we only refactored code to ES6 modules without removing/changing any features. The answer is yes, there should be no breaking changes without major version bumps.

@gka
Copy link
Owner

gka commented Aug 12, 2024

Also, strictly speaking, chroma.js itself is working unchanged, it's just the @typed/chroma-js package that's no longer working, which I have not even created myself but was contributed by someone in the TS community.

This issue has showed me that perhaps it'd be best for chroma.js to be refactored to TS so we don't have to maintain its API surface in two places in the first place.

@aarthificial
Copy link

I believe the problem is with interoperability between CommonJS and ES modules.
Previously it was valid to do:

import {Color} from 'chroma-js';

and

import * as chroma from 'chroma-js'

because of how module.exports is interpreted in the context of ESM.

Now that the package only contains one default ES export with a namespace object, the only way to use it is via:

import chroma from 'chroma-js`

You can see in this issue, for example, that the error is thrown by Vite and happens at the level of import resolution, not type checking (Vite uses esbuild which skips type checking entirely).

@aaronreed708
Copy link
Author

I agree that this is probably an issue between Common JS modules and ES modules. If I fix my .ts files to use import chroma from "chroma-js", I still get the same error.

@aaronreed708
Copy link
Author

Ok, since you believe that this issue isn't your module, I did some more googling. From what I can tell, I have the problem described here: facebook/create-react-app#12700.

And my issue does not occur if I manually make this change outlined in the PR facebook/create-react-app#12352 in my node_modules/react-scripts/config/webpack.config.js.

I'm going to look at doing this approach: facebook/create-react-app#11889 (comment) since modifying react-scripts is not going to fly :-)

@gka
Copy link
Owner

gka commented Aug 14, 2024

Oh, you raised a good point about the non default imports like

import {Color} from 'chroma-js';

I didn't realize that we were breaking them. I'll push an update that fixes this problem.

gka added a commit that referenced this issue Aug 14, 2024
@gka gka closed this as completed in #348 Aug 14, 2024
@gka gka closed this as completed in 60e072d Aug 14, 2024
@gka gka reopened this Aug 14, 2024
@gka
Copy link
Owner

gka commented Aug 14, 2024

can you check if version chroma-js@2.6.1-0 fixes this issue for you? I added the individual exports on top of the default export, like this

import chroma from './src/chroma.js';

import average from './src/generator/average.js';
import bezier from './src/generator/bezier.js';
// ...

Object.assign(chroma, {
    average,
    bezier,
    // ...
});

export default chroma;

export { average, bezier, /* ... */ };

I think you can now import parts of chroma.js directly, e.g.

import { average }  from 'chroma-js';

@aarthificial
Copy link

It fixes some of our imports but not all of them, some modules like src/io/hsl/index.js extend the chroma object without any exports, so things like:

import {hsl} from 'chroma-js'

used to work but now fail.

Fwiw, to me, personally, it's not a big deal. We'll just switch over to using the default namespace. I was worried there was some bigger restructuring when I saw the Color class missing thus the question about semver. The documentation is pretty explicit about using chroma to access these functions so if anything we are to blame for using the lib incorrectly.

@gka
Copy link
Owner

gka commented Aug 17, 2024

alright, thanks for getting back to me.

@gka
Copy link
Owner

gka commented Aug 17, 2024

I think I got it now. In version 3.0.0-0 the import issue should be resolved. Will do some more testing before releasing 3.0.0 out to the world.

@gka
Copy link
Owner

gka commented Aug 21, 2024

I think this can be closed now, right?

@aaronreed708
Copy link
Author

I'm past my issue. Unless you are looking for feedback on your import work on this issue, I'm fine with closing it.

@csmith-em
Copy link

This still appears to be an issue with the default export and the individual exports.

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

Successfully merging a pull request may close this issue.

4 participants