Skip to content

Use get-stdin to read the stdin#710

Merged
fsouza merged 2 commits intomainfrom
use-get-stdin
Apr 12, 2024
Merged

Use get-stdin to read the stdin#710
fsouza merged 2 commits intomainfrom
use-get-stdin

Conversation

@fsouza
Copy link
Copy Markdown
Owner

@fsouza fsouza commented Apr 11, 2024

It's not really a new dependency as prettier already depends on it, but let's bring it in.

I suspect that users are running into weird encoding issues in #694 (and maybe #698).

Rather than spending too much time on this, I'm just taking a shortcut: users confirm that they cannot reproduce the issue in prettier, so let's read stdin the same prettier does :)

fsouza added 2 commits April 11, 2024 21:33
It's not _really_ a new dependency as prettier already depends on it,
but let's bring it in.

I suspect that users are running into weird encoding issues in #694 (and
maybe #698).

Rather than spending too much time on this, I'm just taking a shortcut:
users confirm that they cannot reproduce the issue in `prettier`, so
let's read stdin the same `prettier` does :)
The current state of affairs in prettierd are a bit too tricky when it
comes to modules: it deeply depends on commonjs to dynamically resolve
which prettier to import, and that isn't supported out of the box with
esmodules. It turns out that get-stdin doesn't support cjs, and
typescript is being annoying here, compiling `import` into `require`.

To be honest, migrating to esmodules wouldn't be super tricky (famous
last words): we'd need to implement some of the filesystem navigation to
find the files and then import those with `import()`. The thing is, I'm
not 100% sure that that's trivial, and I allotted some time to work on
prettierd today and that time is running out, and I want to make sure I
fix at least one bug without getting nerdsnipped.

So, rather than trying to migrate everything to esmodules, let's keep it
simple and just vendor `get-stdin`.

Another option I considered was bypassing TS to still use `import()`
with commonjs (some options here:
TypeStrong/ts-node#1290), but that sounds
too painful.
@fsouza fsouza marked this pull request as ready for review April 12, 2024 01:52
@fsouza
Copy link
Copy Markdown
Owner Author

fsouza commented Apr 12, 2024

Will merge this if #698 is green, then release.

@fsouza fsouza merged commit 37bf7b9 into main Apr 12, 2024
@fsouza fsouza deleted the use-get-stdin branch April 12, 2024 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant