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

Fix Vite support #92

Merged
merged 3 commits into from
Jun 7, 2022
Merged

Conversation

kibertoad
Copy link
Collaborator

fix #89

@kibertoad
Copy link
Collaborator Author

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

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2022

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
Copy link

jaclas commented Jun 5, 2022

I checked, everything works, thank you very much!

@jorgebucaran
Copy link
Owner

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

@jaclas
Copy link

jaclas commented Jun 5, 2022

@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
Owner

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

@jaclas
Copy link

jaclas commented Jun 5, 2022

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
Collaborator Author

@jaclas Can you try solution from facebook/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
Copy link

jaclas commented Jun 5, 2022

It helped, the error is no longer there:

obraz

@kibertoad
Copy link
Collaborator Author

kibertoad commented Jun 5, 2022

@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
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
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
Owner

Hmm, this good? 🤔

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

@kibertoad
Copy link
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
Copy link

jaclas commented Jun 6, 2022

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
Owner

Thanks for the info, @jaclas. 👍

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

@kibertoad
Copy link
Collaborator Author

kibertoad commented Jun 6, 2022

@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
Copy link

jaclas commented Jun 6, 2022

@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
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
Copy link

jaclas commented Jun 6, 2022

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
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
Copy link

so1ve commented Dec 29, 2022

@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