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

Issue with Colorjs.io that seems to lead back to Vanilla Extract #899

Closed
2 tasks done
ghost opened this issue Nov 3, 2022 · 6 comments
Closed
2 tasks done

Issue with Colorjs.io that seems to lead back to Vanilla Extract #899

ghost opened this issue Nov 3, 2022 · 6 comments
Labels
integration Issue related to the integration package, potentially affects multiple plugins priority-medium Medium priority issue

Comments

@ghost
Copy link

ghost commented Nov 3, 2022

Describe the bug

I have an issue where the Colorjs.io library toString() method outputs a wrong color format when used inside *.css.ts files.

I filed this issue originally here and there's some further information, but given the method seems to otherwise work well in pure node, it may be related to how VE builds the *.css.ts files. Any ideas what could be happening?

Reproduction

https://github.com/mystrdat/colorjs-vanilla-test

System Info

System:
    OS: Linux 6.0 Fedora Linux 37.20221101.n.0 (Silverblue)
    CPU: (12) x64 AMD Ryzen 5 5600X 6-Core Processor
    Memory: 24.92 GB / 31.28 GB
    Container: Yes
    Shell: 3.5.0 - /usr/bin/fish
  Binaries:
    Node: 16.17.1 - ~/.local/share/nvm/v16.17.1/bin/node
    npm: 8.15.0 - ~/.local/share/nvm/v16.17.1/bin/npm
  npmPackages:
    @vanilla-extract/css: ^1.9.1 => 1.9.1 
    @vanilla-extract/vite-plugin: ^3.6.1 => 3.6.1 
    vite: ^3.2.0 => 3.2.2

Used Package Manager

npm

Logs

No response

Validations

@ghost ghost added the pending triage label Nov 3, 2022
@rothsandro
Copy link

rothsandro commented Nov 3, 2022

Just debugged a bit in both the browser and node (with no knowledge of how VE works internally). The difference between browser and node is in ColorSpace.getFormat when calling processFormat() (ref):

getFormat (format) {
  // ...
  if (ret) {
    ret = this.#processFormat(ret); // We end up here
    return ret;
  }
}

In node with VE the compiled code looks like this:

ret = __privateMethod(this, _processFormat, __create).call(this, ret);

In the browser the processFormat() is called as expected. In node there is a strange behavior. The third argument __create is the function that gets called (returned by __privateMethod). Now the funny thing is that __create is declared and initialized twice in the same file, once for the processFormat function and once for the getPath function - and we end up in the latter one instead of processFormat (which then returns something different which than later breaks everything). The file looks like this:

var _processFormat, __create, _path, _getPath, __create; 

// ...

__create = function(format) {
  // Code of processFormat()
}

// ...

__create = function() {
 // Code of getPath()
}

Assuming that the file was transformed by esbuild (as Vite uses esbuild, but I don't know if there is any other magic going on): this was actually a bug in esbuild that was fixed over one year ago evanw/esbuild#1424


That's all I've found out so far. It's time to call it a day 😴

@ghost
Copy link
Author

ghost commented Nov 3, 2022

Thank you very much for the insight! If that's the case the bug was fixed in esbuild@0.12.17, yet Vite uses ^0.15.9 https://github.com/vitejs/vite/blob/main/packages/vite/package.json#L61 😵‍💫

@rothsandro
Copy link

Just debugged further: Vite does not transform the file with esbuild but calls the transform function of VE, and VE itself transforms it using esbuild. But VE uses its own esbuild version located in node_modules/@vanilla-extract/integration/node_modules/esbuild.

And the @vanilla-extract/integration package uses esbuild 0.11.x, so the bug still exists in this version. So I guess updating the esbuild version in the integration package should fix the bug.

@ghost
Copy link
Author

ghost commented Nov 4, 2022

That makes sense given the bug only manifests in *.css.ts files and not otherwise in the Vite stack. @markdalgleish would it be possible to update esbuild in the integration package to squash this? Many thanks again for debugging this @rothsandro ❤️

@ghost
Copy link
Author

ghost commented Nov 9, 2022

Could anyone from the VE team look into this? IMO this bug opens a potentially infinite volume of bizarre issues and should probably be treated asap.

@askoufis askoufis added esbuild Issue is related to esbuild priority-high High priority issue priority-medium Medium priority issue integration Issue related to the integration package, potentially affects multiple plugins and removed pending triage priority-high High priority issue esbuild Issue is related to esbuild labels Jan 17, 2023
@askoufis
Copy link
Contributor

Fixed in #973

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration Issue related to the integration package, potentially affects multiple plugins priority-medium Medium priority issue
Projects
None yet
Development

No branches or pull requests

2 participants