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

Use get-stdin to read the stdin #710

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Use get-stdin to read the stdin #710

merged 2 commits into from
Apr 12, 2024

Conversation

fsouza
Copy link
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 :)

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
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
6 checks passed
@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.

None yet

1 participant