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

.display and .toString different behavior on nextjs in dev and build mode #260

Closed
star-over opened this issue Dec 11, 2022 · 11 comments · Fixed by #421
Closed

.display and .toString different behavior on nextjs in dev and build mode #260

star-over opened this issue Dec 11, 2022 · 11 comments · Fixed by #421

Comments

@star-over
Copy link

star-over commented Dec 11, 2022

Trying to use color.display({"format": "hex"}) as well as .toString({"format": "hex"}) in nextjs v13.
In dev it works fine, outputs #111 for example, but in prod mode it outputs color(srgb 0.0667 0.0667 0.0667) when running bundle after next build.
So I use this method to get the same result in dev and in prod mode Color.defaults.display_space.formats.hex.serialize(color.coords).
Is there any "correct" solution to output hexadecimal representation of colour (#123456)?

@LeaVerou
Copy link
Member

This sounds very similar to an issue we had recently and fixed in #239
Could you please make sure you’re using v0.4.2 of Color.js?

@star-over
Copy link
Author

Pretty shure.
"colorjs.io": "^0.4.2" - record in package.json

@nick-inkeep
Copy link

@LeaVerou any update on this? Facing same issue.

@LeaVerou
Copy link
Member

Hey, sorry this slipped through the cracks. Could you make a reduced testcase that I could look at and play with (e.g. on stackblitz or something)?

@sarah-inkeep
Copy link

sarah-inkeep commented Jul 20, 2023

Hi @LeaVerou! I made a reduced test case here. When it's running in dev mode (next dev) you'll see the color is in the expect hex format, but if you run next build && next start and then refresh, it outputs as color(srgb 0.3216 0.1843 0.7882).

@jh3y
Copy link

jh3y commented Aug 8, 2023

Can confirm, having this issue too, albeit slightly differently.

let color = new Color('hsl(0 100% 50%)')
color.to('p3').toString() // Dev = color(display-p3 0.9175 0.2003 0.1386)
color.to('p3').toString() // Prod = color(p3 0.9175 0.2003 0.1386)

Works fine for other formats I've found 🤔

@lloydk
Copy link
Collaborator

lloydk commented Feb 10, 2024

I took a look at this, the reduced test case was super helpful.

When the test app is run in dev mode the code for the page is run in the browser and works correctly.

In production mode (next build && next start) Next.js generates static pages and the code for the page is run as part of the build process and does not run correctly.

The color.js code that is failing is the assignment of the format variable:

format = color.space.getFormat(format)
?? color.space.getFormat("default")
?? ColorSpace.DEFAULT_FORMAT;

If you print out the value of the format variable (pay attention to the location of the console.log statement) the value of format is "hex" which is the value that was passed into the serialize function.

	format = color.space.getFormat(format)
	       ?? color.space.getFormat("default")
	       ?? ColorSpace.DEFAULT_FORMAT;

	inGamut ||= format.toGamut;

	let coords = color.coords.slice(); // clone so we can manipulate it

	if (inGamut && !checkInGamut(color)) {
		// FIXME what happens if the color contains NaNs?
		coords = toGamut(clone(color), inGamut === true ? undefined : inGamut).coords;
	}

	console.log(`format is ${format}`);

If you move the console.log statement just below the assignment of the format variable the code runs correctly.

	format = color.space.getFormat(format)
	       ?? color.space.getFormat("default")
	       ?? ColorSpace.DEFAULT_FORMAT;
	console.log(`format is ${format}`);

	inGamut ||= format.toGamut;

If you access a property of format right after the assignment the code runs correctly.

	format = color.space.getFormat(format)
	       ?? color.space.getFormat("default")
	       ?? ColorSpace.DEFAULT_FORMAT;

	let x = format.type;

If you replace the nullish coalescing operators with if/else statements the code runs correctly.

	format = color.space.getFormat(format);

	if (!format) {
		format = color.space.getFormat("default");
	}
	else {
		if (!format) {
			format = ColorSpace.DEFAULT_FORMAT;
		}
	}

This also works and maybe should be the preferred workaround?

	format = color.space.getFormat(format) ?? color.space.getFormat("default");

	if (!format) {
		format = ColorSpace.DEFAULT_FORMAT;
	}

TLDR: The issue appears to be Next.js bug but there are some ways to work around the issue.

@LeaVerou
Copy link
Member

Thank you so much for tracking it down @lloydk! Should we close this issue then?

@lloydk
Copy link
Collaborator

lloydk commented Feb 10, 2024

Thank you so much for tracking it down @lloydk! Should we close this issue then?

Up to you, we can either close the issue or I can create a PR for one of the work arounds.

@svgeesus
Copy link
Member

Maybe we should have a FAQ listing these sorts of known issues and workarounds?

@lloydk
Copy link
Collaborator

lloydk commented Feb 10, 2024

Changing the code from

	inGamut ||= format.toGamut;

	let coords = color.coords.slice(); // clone so we can manipulate it

to

	let coords = color.coords.slice(); // clone so we can manipulate it

	inGamut ||= format.toGamut;

seems to work for me so I'll create a PR for that change.

lloydk added a commit to lloydk/color.js that referenced this issue Feb 10, 2024
Workaround for a Next.js bug when generating a production build
of a Next.js project using Color.js.

See this Github issue for an analysis of the bug:
color-js#260
LeaVerou pushed a commit that referenced this issue Feb 11, 2024
* Add a workaround for Next.js production build bug

Workaround for a Next.js bug when generating a production build
of a Next.js project using Color.js.

See this Github issue for an analysis of the bug:
#260

* Add comment for workaround
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 a pull request may close this issue.

7 participants