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

Would you accept a PR that converts this project to build with TypeScript? #6286

Open
octogonz opened this issue Jul 10, 2019 · 23 comments
Open
Labels
status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:typechecking Issues and PRs related to using TypeScript's JSDoc syntax to typecheck the codebase

Comments

@octogonz
Copy link

I've been studying the Prettier code and considering contributing some fixes.

However, I am finding the source code to be rather difficult to follow, because there are no type declarations anywhere. For example, here's a typical function:

function isWithinParentArrayProperty(path, propertyName) {
  const node = path.getValue();
  const parent = path.getParentNode();

  if (parent == null) {
    return false;
  }

  if (!Array.isArray(parent[propertyName])) {
    return false;
  }

  const key = path.getName();
  return parent[propertyName][key] === node;
}

For a newcomer, it takes some effort to figure out:

  • What kind of thing is path?
  • What kind of thing is parent?
  • Which of these things is allowed to be null (or undefined)?

The AST data structure in particular is fairly complicated. I recently worked on another project that used Recast, and there the TypeScript validation caught a lot of mistakes. At every step of the AST tree, the type information clearly specifies what kind of nodes are allowable, and the IntelliSense shows you the available properties.

Converting the Prettier code base to TypeScript is obviously a nontrivial amount of work. But if someone from the community volunteered to do this, would you accept such a PR?

@alexander-akait
Copy link
Member

alexander-akait commented Jul 10, 2019

I don't think we need typescript here, just my opinion, i love typescript, but not here. He have a lot of issues what should be solved, rewriting will take a long time so better focus on bugs/features.

@duailibe
Copy link
Member

duailibe commented Jul 10, 2019

Thanks for the offer! We have considered it in the past, but one restriction we value is being able to install Prettier directly from GitHub: this enables our users to be unblocked in case they are affected with a bug, or need a feature, until we release a new version.

It also lowers the barrier for people to fork Prettier. In that case, they don't need to publish a new package to use their fork in their projects.

This restriction makes it hard to use TypeScript (or any other "preprocessor").

@octogonz
Copy link
Author

This restriction makes it hard to use TypeScript (or any other "preprocessor").

@duailibe Wouldn't these same considerations apply generally to any project that uses any sort of transpiler (Jest, Yarn, Angular, etc.)? Those projects seem to have concluded that readability and validation are important enough concerns to be worth the extra overhead.

It also lowers the barrier for people to fork Prettier. In that case, they don't need to publish a new package to use their fork in their projects.

This could be solved by committing the build output, and then pushing it to a GitHub release branch, right?

@dasa
Copy link

dasa commented Jul 10, 2019

Maybe using TypeScript on the js source code like this would be useful: https://devblogs.microsoft.com/typescript/how-to-upgrade-to-typescript-without-anybody-noticing-part-1/
https://devblogs.microsoft.com/typescript/how-to-upgrade-to-typescript-without-anybody-noticing-part-2/

@alexander-akait
Copy link
Member

alexander-akait commented Jul 10, 2019

@dasa yep, using comments for typescript is good idea for prettier (webpack do same)

@duailibe
Copy link
Member

duailibe commented Jul 11, 2019

@octogonz

Wouldn't these same considerations apply generally to any project that uses any sort of transpiler (Jest, Yarn, Angular, etc.)?

Yes, so I'd guess they have a different set of trade-offs than Prettier.

This could be solved by committing the build output, and then pushing it to a GitHub release branch, right?

I think so.

@dasa Yes, that's an option. I opened a PR doing that with Flow comments, but I gave up in the end. Nobody did the same with JSDoc yet.
I'm also under the impression that this is less powerful than using TS syntax, so I thought it might be less useful. Didn't investigate it. Happy to learn more about it.

@thorn0
Copy link
Member

thorn0 commented Jul 24, 2019

I'm also under the impression that this is less powerful than using TS syntax, so I thought it might be less useful. Didn't investigate it. Happy to learn more about it.

We've already discussed this concern in that PR.

@octogonz
Copy link
Author

Such gymnastics may not really be necessary. Let's say that converting all the unit tests is relatively unimportant, and could be done later, incrementally. The subtree that matters is prettier/src, since it contains the public APIs and important formatting logic.

I counted 110 .js files in that folder. If the Prettier project wants to move to TypeScript, we could throw humans at the problem and convert the files manually. Many projects have done this sort of conversion before. It's well understood and can go pretty fast. The work is rather fun. 110 files is small enough to complete in a matter of weeks. The conversion could be validated by diffing the TypeScript compiler output against the original sources (since the initial conversion would aim to add type annotations without altering semantics in any way).

So instead of "how would we accomplish this?" the main question is: "If someone from the community volunteered to do this, would such a PR be accepted?"

@Shinigami92
Copy link
Member

Shinigami92 commented Jul 25, 2019

I'm one of these humans 🙋‍♂️

The current "problem" is that prettier can also be used directly as a dependency on the GitHub repo.
So if I unterstand correctly some depends on the src folder?!

@thorn0
Copy link
Member

thorn0 commented Jul 25, 2019

Many projects have done this sort of conversion before. It's well understood and can go pretty fast.

It's not always easy 'to capture highly dynamic JS behavior in static types'. Prettier with all the different AST formats it works with seems like one of those projects with highly dynamic JS behavior. TS might work for it, but the conversion might not be exactly pretty fast. I wish I was wrong.

@alexander-akait
Copy link
Member

Unfortunately, you are right

@Skillz4Killz
Copy link

Skillz4Killz commented Dec 13, 2019

Thanks for the offer! We have considered it in the past, but one restriction we value is being able to install Prettier directly from GitHub: this enables our users to be unblocked in case they are affected with a bug, or need a feature, until we release a new version.

It also lowers the barrier for people to fork Prettier. In that case, they don't need to publish a new package to use their fork in their projects.

This restriction makes it hard to use TypeScript (or any other "preprocessor").

Add a "prepack" script in package.json that does tsc so users can use the Github directly or even fork and use their github even if it is a Typescript repo.

@Shinigami92
Copy link
Member

Add a "prepack" script in package.json that does tsc so users can use the Github directly or even fork and use their github even if it is a Typescript repo.

Some questions:

  • Does someone need TypeScript in their own package.json for this to work?
  • According to the documentation https://docs.npmjs.com/misc/scripts
    Will this run on the consumer side? Or on pack/publish to npm? Or both?
  • What could be a sideeffect?
  • So I'm a little bit afraid of installing/running something on consumer side on install 😬
  • ... Could typescript be a peerDependency in prettier?

@octogonz
Copy link
Author

Executing the source code seems like a strange requirement to impose. Other TypeScript/Babel projects don't worry about this. Building code is a standard part of professional software development. If our goal is to make it easier for people to improve Prettier, then TypeScript's improved validation and readability seem like a more important requirement.

If a person is smart enough to modify a complex tool like Prettier, it's probably reasonable to assume they will know how to (1) publish to a private NPM registry or (2) commit their compiled output to a Git branch somewhere.

@lydell
Copy link
Member

lydell commented Dec 16, 2019

Maybe we could automatically publish alpha versions of Prettier on each push to master to avoid the whole “install from github compatible” things 🤷‍♂

@waynevanson
Copy link

A strategy that might work would be to partially adopt typescript to the build.

If this project builds correctly javascript to javascript with the typescript compiler. There are so many tests in here that errors would be caught before releases.

For the dynamic language feature concerns: overloads, generics and mappable types allow flexible generation of types. I'd be keen to help out on something like this.

@Shinigami92
Copy link
Member

The last time I looked at adding JSDoc typings, it was incredibly difficult to find some types, especially for large complex objects like Doc
This could even be impossible to type with JSDoc...

I would appreciate to rewrite prettier in TypeScript, but there are statements like "This repo can be added as a dependency directly from the master branch" and so on (I've never used such black magic myself)

I don't know the current opinion

@waynevanson
Copy link

@Shinigami92 Someone suggested that perhaps that the master branch could be the built branch, with a different branch as the development branch. That could remove the black magic headache.

@Shinigami92
Copy link
Member

I would be happy with that. So the master branch would be the builded endresult and the development branch uses github actions to automatically build and push into master?

So master would be something like gh-pages branch?! ^^

@waynevanson

This comment was marked as outdated.

@Shinigami92
Copy link
Member

One of the main TS leads and contributer of this tsconfig.json... It's me
The current state was to first introduce typings with JSDoc

@octogonz
Copy link
Author

octogonz commented May 21, 2020

The original intent of this issue was to decide whether Prettier should join other popular tools that have moved to TypeScript. (How to perform the conversion is a separate topic.)

Since it's been it a year with no consensus, let's change the question:

What is the way to reach a yes/no decision regarding TypeScript? A community vote with a specified end date? An authoritative statement from one of the maintainers?

@thorn0 thorn0 added the type:typechecking Issues and PRs related to using TypeScript's JSDoc syntax to typecheck the codebase label Sep 6, 2022
@JounQin
Copy link
Member

JounQin commented Jan 18, 2024

Can we reconsider this in year 2024? 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:typechecking Issues and PRs related to using TypeScript's JSDoc syntax to typecheck the codebase
Projects
None yet
Development

No branches or pull requests

10 participants