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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make sur it works in browser where tty package isnt available #64

Closed
wants to merge 1 commit into from
Closed

Conversation

WNemencha
Copy link

No description provided.

@jorgebucaran
Copy link
Owner

I'm curious. How are you using Colorette in the browser?

@WNemencha
Copy link
Author

WNemencha commented Sep 9, 2021

It's used in our dependencies by some tools (postcss i guess) and postcss is used by sanitize-html, which we do use to sanitize markdown ^^. The fact that its here in the bundle was throwing like: tty.isatty is not a function. I've tried to "polyfill" it by setting window.tty.isatty = () => false, but didn't worked, so here's the working fix (can confirm, tried with package.json#resolutions on my branch)

PS: I hope this message does not feels harsh, whishing u a great evening ;)

@jorgebucaran
Copy link
Owner

Shouldn't this be handled by your bundler or compiler?

@kibertoad
Copy link
Collaborator

@jorgebucaran How can webpack handle Node context not being there?

@jorgebucaran
Copy link
Owner

I really don't know. What about a global shim? Colorette shouldn't be in the OP's bundle to start with. :trollface:

@kibertoad
Copy link
Collaborator

SSR is one possible option. Serverless is another.
I'm not sure there is a good working solution, and what is the issue with handling gracefully on colorette side? doesn't it come with zero performance impact?

@WNemencha
Copy link
Author

Agree with @kibertoad, that's just a branch, and branches are cheap. Configuration of bundle processor in the other side isn't about branches, thus costly ^^

@kibertoad
Copy link
Collaborator

@WNemencha I wonder why additional check is necessary, though. Wouldn't tty be undefined already? Also is 'process' available? I would expect that to fail too.

@WNemencha
Copy link
Author

WNemencha commented Sep 21, 2021

@kibertoad The tty from the import is undefined if u have a commonjs bundle, when using ESM, it will still returns a { __esModule: true } i believe, so the tty && will still evaluate to true, and then {}.isatty() call will obviously throws :(
Also, modern browsers supports process.browser: boolean as a mean to check if in a browser

@LeoniePhiline
Copy link

LeoniePhiline commented Sep 21, 2021

@WNemencha This is exactly #67

Proposal:

tty?.isatty?.(1)

This works even if bundlers proxy / mock the module but not the function. (e.g. in vite)

@jorgebucaran jorgebucaran added the bug Something isn't working label Sep 21, 2021
@jorgebucaran
Copy link
Owner

@WNemencha Also, modern browsers supports process.browser...

Do you mean bundlers add special support for this? I'm not aware of any process.browser API in browsers or in node, is this an old thing? We are currently importing process explicitly, but maybe we don't have to?

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.

None yet

4 participants