Skip to content

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

Closed
WNemencha wants to merge 1 commit into
jorgebucaran:mainfrom
WNemencha:patch-1
Closed

fix: make sur it works in browser where tty package isnt available#64
WNemencha wants to merge 1 commit into
jorgebucaran:mainfrom
WNemencha:patch-1

Conversation

@WNemencha

Copy link
Copy Markdown

No description provided.

@jorgebucaran

Copy link
Copy Markdown
Owner

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

@WNemencha

WNemencha commented Sep 9, 2021

Copy link
Copy Markdown
Author

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
Copy Markdown
Owner

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

@kibertoad

Copy link
Copy Markdown
Collaborator

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

@jorgebucaran

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

WNemencha commented Sep 21, 2021

Copy link
Copy Markdown
Author

@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

LeoniePhiline commented Sep 21, 2021

Copy link
Copy Markdown

@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
Copy Markdown
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.

4 participants