Skip to content

Fix Vite support#92

Merged
jorgebucaran merged 3 commits into
jorgebucaran:mainfrom
kibertoad:fix/vite-support
Jun 7, 2022
Merged

Fix Vite support#92
jorgebucaran merged 3 commits into
jorgebucaran:mainfrom
kibertoad:fix/vite-support

Conversation

@kibertoad

Copy link
Copy Markdown
Collaborator

fix #89

@kibertoad kibertoad requested a review from jorgebucaran June 4, 2022 13:32
@kibertoad

Copy link
Copy Markdown
Collaborator Author

@jaclas Can you check if this branch fixes your issue?

@codecov-commenter

codecov-commenter commented Jun 4, 2022

Copy link
Copy Markdown

Codecov Report

Merging #92 (e698b72) into main (106748e) will not change coverage.
The diff coverage is 100.00%.

❗ Current head e698b72 differs from pull request most recent head 5e8f161. Consider uploading reports for the commit 5e8f161 to get more accurate results

@@            Coverage Diff            @@
##              main       #92   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          146       146           
=========================================
  Hits           146       146           
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 106748e...5e8f161. Read the comment docs.

@jaclas

jaclas commented Jun 5, 2022

Copy link
Copy Markdown

I checked, everything works, thank you very much!

@jorgebucaran

Copy link
Copy Markdown
Owner

Thank you, @jaclas. Just curious, but how did you test that this branch fixed your issue?

@jaclas

jaclas commented Jun 5, 2022

Copy link
Copy Markdown

@jorgebucaran Damn... I installed colorette with npm, did a quick test, saw the color changes in the console (bash) and wrote that it was ok. Sorry for the rush, didn't want to block the issue and hence too superficial check.

@jorgebucaran

Copy link
Copy Markdown
Owner

Okay, so are we positive that this fixes the Vite issue? Just want to make sure.

@jaclas

jaclas commented Jun 5, 2022

Copy link
Copy Markdown

Unfortunately not, I modified the code bundled about by Vite for the latest changes from Colorrette and there is still an error:

obraz

@kibertoad

Copy link
Copy Markdown
Collaborator Author

@jaclas Can you try solution from react/create-react-app#12212 and report back if it helps?

Namely adding

if (!('process' in window)) {
  // @ts-ignore
  window.process = {}
}

into the beginning of the file

@jaclas

jaclas commented Jun 5, 2022

Copy link
Copy Markdown

It helped, the error is no longer there:

obraz

@kibertoad

kibertoad commented Jun 5, 2022

Copy link
Copy Markdown
Collaborator Author

@jorgebucaran I don't think it's possible to have version that works both with Node and with browser, and also is an ESM module (because that one executes in strict mode and does not allow referencing undefined variables). How would you like to proceed?

@jorgebucaran

Copy link
Copy Markdown
Owner

Would import process from "node:process" help?

Can anybody help me understand how browser support should work? How do you use this package in a browser? And what is the expected behavior (other than not seeing an uncaught error)?

@jorgebucaran jorgebucaran added the bug Something isn't working label Jun 5, 2022
@kibertoad

Copy link
Copy Markdown
Collaborator Author

No, because it is failing in non-node environments where node package is not going to be present either.
Not sure where it is used in browser, I assume some of the deps are colouring their console with colorette, hence things break down.
Fixing support universally would require making colorette a CJS module (so that it no longer crashes on referencing undeclared variables)

@jorgebucaran

Copy link
Copy Markdown
Owner

Hmm, this good? 🤔

const proc = typeof process === "undefined" ? {} : process

@kibertoad

Copy link
Copy Markdown
Collaborator Author

@jorgebucaran I think any reference to non-defined variable will break it. you can try running locally and replace process with xyz

@jaclas

jaclas commented Jun 6, 2022

Copy link
Copy Markdown

I am using colorette in a backend project in node. There I made my own wrapper for a logger that also uses colorette. Now I started working in SvelteKit and just wanted to move my logger wrapper to that environment, unfortunately I ran into a problem and had to abandon colorette. SvelteKit works on both sides, the backend and the frontend, in the backend everything colors nicely (I like it a lot), and already in the browser it would be enough if it didn't crash, I don't know if the colors are even available in the browser console.

@jorgebucaran

Copy link
Copy Markdown
Owner

Thanks for the info, @jaclas. 👍

@kibertoad #92 (comment) seems to work for me.

@kibertoad

kibertoad commented Jun 6, 2022

Copy link
Copy Markdown
Collaborator Author

@jaclas Could you please try latest version of this branch? It should fix the issue for real now, would be great to confirm :D

@jaclas

jaclas commented Jun 6, 2022

Copy link
Copy Markdown

@kibertoad Okay, just give me a hint how should I install this version? Should I clone your repo or is it possible via npm?

@kibertoad

Copy link
Copy Markdown
Collaborator Author

@jaclas You can use npm: https://stackoverflow.com/questions/39732397/install-specific-branch-from-github-using-npm

But actually easiest way to try would be just copy-paste whole index.js from the branch into your local node_modules version :D

@jaclas

jaclas commented Jun 6, 2022

Copy link
Copy Markdown

Thx for tip with npm. It will come in handy on other occasions.
I have now copied the file as you advised and it seems to be ok:

obraz

@kibertoad

Copy link
Copy Markdown
Collaborator Author

@jaclas Thank you for checking this!
@jorgebucaran We are good to merge then :)

@jorgebucaran jorgebucaran merged commit e83603a into jorgebucaran:main Jun 7, 2022
@so1ve

so1ve commented Dec 29, 2022

Copy link
Copy Markdown

@jaclas Actually you can use define:

export default defineConfig({
  define: {
    'process.env': {}
  }
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SvelteKit + colorette

5 participants