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

detect-intent is buggy and we cannot update it #5664

Closed
zkochan opened this issue Nov 21, 2022 · 3 comments · Fixed by #5666
Closed

detect-intent is buggy and we cannot update it #5664

zkochan opened this issue Nov 21, 2022 · 3 comments · Fixed by #5666
Milestone

Comments

@zkochan
Copy link
Member

zkochan commented Nov 21, 2022

    Sorry I had been busy for a while. My apologies. Given your closing this issue, are you no longer interested in gradually migrating pnpm to hybrid CommonJS/ES Module? The detect-indent version currently being used is badly buggy (see https://github.com/sindresorhus/detect-indent/pull/34) but the fixed version is ESM-only, so I don't see a way forward without modifying pnpm so that at least in some places it performs ESM imports (or abandoning detect-indent, but I am not sure what would replace it). I can work on a PR now that changes just read-project-manifest to use ESM imports but also provide a CommonJS module, as a proof-of-concept, if you'd still be willing to consider merging it. 

I guess if you are now against exploring ESM imports altogether, I could alternatively make a PR that would supply an internal, written-from-scratch replacement for detect-indent. Would you be willing to consider that alternative if you are against ESM?

Thanks for letting me know which way to try to go with this.

Originally posted by @gwhitney in #3139 (comment)

@zkochan
Copy link
Member Author

zkochan commented Nov 21, 2022

I guess if you are now against exploring ESM imports altogether, I could alternatively make a PR that would supply an internal, written-from-scratch replacement for detect-indent. Would you be willing to consider that alternative if you are against ESM?

Could we just convert detect-intent to commonjs using babel?

@gwhitney
Copy link
Contributor

Hmm, the suggestions that I've received are to fork detect-indent and create a commonjs version, or to use babel. I don't know the first thing about babel, and this doesn't seem worth forking a package with the consistency issues that creates. What detect-indent does and how it does it are so minor that for me the path of least resistance would be just to contribute totally fresh code that does the same thing. It would just be a single, not-all-that-long function in read-project-manifest. Would such a contribution, removing the dependency on detect-indent altogether, be considered for merging? Thanks for letting me know.

@zkochan
Copy link
Member Author

zkochan commented Nov 21, 2022

you may just fork https://github.com/sindresorhus/detect-indent

change it to commonjs

publish it under your scope to npmjs

and we'll use your fork instead.

zkochan pushed a commit that referenced this issue Nov 23, 2022
@zkochan zkochan added this to the v7.17 milestone Dec 1, 2022
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 a pull request may close this issue.

2 participants