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 a monorepo and separate out Prettier’s components #3511

Closed
j-f1 opened this issue Dec 17, 2017 · 23 comments
Closed

Use a monorepo and separate out Prettier’s components #3511

j-f1 opened this issue Dec 17, 2017 · 23 comments
Assignees
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:infra Issues about CI, publishing to npm, or similar type:meta Issues about the status of Prettier, or anything else about Prettier itself

Comments

@j-f1
Copy link
Member

j-f1 commented Dec 17, 2017

This comes from the Prettier 2.0 issue

@j-f1:

Would it make sense to extract the parser and printer files to separate packages? i.e. you’d do npm install --save-dev prettier @prettier/plugin-js @prettier/plugin-markdown prettier-plugin-python. This would mean that Prettier doesn’t have to include all languages it supports by default, and that folks could release new parsers before they’re completely ready for production. Plus, this would let the Prettier core stay small and testable.

@azz:

It's possible but would require some thought about what would be in the plugin API. doc-builders, utils, etc. Multiparser would require an API and it would (potentially) limit the integrations you could do between different plugins.

@lydell:

Regarding extracting the printer, see #2029.

@j-f1:

Multiparser would require an API and it would (potentially) limit the integrations you could do between different plugins.

How about this?

const { parseEmbedded } = require('prettier')

module.exports = function parser(code, ...) {
  return concat([
    // ...
    parseEmbedded(code, languageHint)
    // ...
  ])
}

(where languageHint is the fenced code block title, or the type attribute of the <script> or <style> etc.)

@TrySound:

For me seamless installing of prettier is the greatest feature. I don't care big package size since it's zero dependencies and usually not used as subdependency. Splitting package will require more configuration/installing and it's IMO worse DX.

@lydell:

If we split things up, it will be a huge task. I'd say we don't do anything such for 2.0. Better to focus on the easier stuff. (I also agree with @TrySound that it is super nice that Prettier is so easy to install.)

@duailibe:

I've actually thought about splitting some parts of the project in separate packages (like a monorepo) to force us to have clear boundaries between the different parts of the code (even using @azz's ESLint rules for monorepos), like CLI, formatting, config, etc.. we do have a separation of files today, but it's not clear what are the dependencies of each part. And we'd still ship a single package, so no changes at all for users.

After that it would be more easy to create different bundles (e.g. for web) as we'd know exactly how to use only specific parts (i.e. only include formatting parts, not the CLI, config, etc.)

@j-f1:

Plus that would make it easier to create custom parsers/printers, since the API is already used internally.

@duailibe:

Regardless, we should probably create a separate issue for this since it would be only an internal change and so unrelated to 2.0

@duailibe duailibe added status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:infra Issues about CI, publishing to npm, or similar type:meta Issues about the status of Prettier, or anything else about Prettier itself labels Dec 17, 2017
@azz
Copy link
Member

azz commented Dec 19, 2017

I think there are significant advantages to sticking to one installable on npm for "core prettier" things (everything in this repo), but being able to plug Prettier with new parsers and printers is valuable. I've been toying around with a refactor sketch to force an API on the doc-builders and co. Feedback appreciated!


Directory Structure

src/
  cli/
    ...
  config/
    resolve-config.js
    ...
  builder/          # external api
    builders.js
    util.js
  language-js/      # babylon/flow/typescript
    index.js
    parser.js
    printer.js
    embed.js
  language-css/     # css/scss/less
    index.js
    parser.js
    printer.js
    embed.js
  ...
  main/
    fast-path.js
    index.js
    ast-to-doc.js
    doc-to-string.js
    doc-debug.js

Language Plugin API

Each of the language-* directories, as well as external plugins will a export parse, print and (optionally) embed functions:

  • export function parse(text, options): AST {}

    Parse code into an AST.

  • export function print(path, print, options): IRDoc {}

    Convert an AST node (represented by path) into a Prettier IR Doc. Doc builders such as concat, etc, will be available to the public via require("prettier/builder"). Call print to print child nodes.

  • export function embed(path, print, format, options): IRDoc | null {}

    Given a path from this parser, determine if another parser should be applied. When switching to a different parser, return format(text, { parser: "..." }).

Additionally they will export options and languages metadata:

  • export const languages = [
      { name: "python", ... }
    ];

    This is the same format as getSupportInfo().languages

  • export const options = [
      { name: "myCustomOption", ... }
    ]

    This is the same format as getSupportInfo().options (feat(support-info): add options field #3433)

Configuring Language Plugins

Module names will be resolved using Node.js module resolution, so both absolute and relative paths will work.

.prettierrc:

{
  "plugins": ["prettier-ruby", "./prettier-csharp"]
}

CLI

prettier --plugin prettier-ruby --plugin ./prettier-csharp --write

We could go one step further and use lerna, converting each of the above directories in src/ to a package, and bundling them all into "prettier" before publishing, but it may be overkill.

@j-f1
Copy link
Member Author

j-f1 commented Dec 19, 2017

Why does Prettier bundle its dependencies? It would make more sense IMO to have both the Prettier core and Prettier plugins require('prettier-builders').

@TrySound
Copy link
Contributor

To be fast?

@duailibe
Copy link
Member

Here's some reference: #735

@duailibe
Copy link
Member

And here too: #1640 (comment)

@duailibe
Copy link
Member

@azz I liked the language plugin API, I didn't understand the configuration part. Why is that necessary? Community plugins?

@azz
Copy link
Member

azz commented Dec 19, 2017

Yes, only need config for separate packages.

@duailibe
Copy link
Member

In that case, can we force lazy loading the parsers (or expose hooks for when to load) on those unofficial plugins?

@azz
Copy link
Member

azz commented Dec 19, 2017

They'd be loaded at the same time a regular parser will be loaded, the resolution will just be different.

@azz
Copy link
Member

azz commented Dec 19, 2017

I'm going to have a crack at this over the next couple of days.

@azz azz self-assigned this Dec 19, 2017
@duailibe
Copy link
Member

duailibe commented Dec 19, 2017

@azz I mean nothing will prevent someone from including a require('XYZ-parser') in language-XYZ/index.js. A plugin.load + documentation might make more clear.

I'm going to have a crack at this over the next couple of days.

Nice! Let me know if you need help, I've started some experimenting with loading parsers for #3296 but this should unblock that anyway :-)

This was referenced Dec 19, 2017
@azz
Copy link
Member

azz commented Dec 20, 2017

Need a way for a plugin (e.g. our own javascript support) to support multiple parsers that map to one printer.

E.g. we have parsers:

  • babylon
  • flow-parser
  • typescript-eslint-parser

And soon babylon's typescript plugin will be compatible.

Which all output a similar AST. We have two "languages" here: javascript and typescript.

All of them map to the same printer. (let's call it estree after the AST it serializes)

Could it look something like this?

export const languages = [
  {
    name: 'javascript',
    // first would be the default?
    parsers: ['babylon', 'flow'],
    astFormat: 'estree',
  },
  {
    name: 'typescript',
    parsers: ['typescript-eslint', ['babylon', { extraPlugins: ['typescript'] }]],
    astFormat: 'estree',
  },
];

export const parsers = {
  babylon: parseWithBabylon,
  flow: parseWithFlow,
  'typescript-eslint': parseWithTsep,
};

export const printers = {
  estree: {
    print: printAstToDoc,
    embed: embedFromEstree,
  },
};

Then we can support a generic --parser.javascript flow option for the current --parser flow use case.

@j-f1
Copy link
Member Author

j-f1 commented Dec 20, 2017

👍 That API looks pretty cool.

Also, if I were to make my own JS variant, I could do something like this:

export const languages = [
  {
    name: 'sumatrascript',
    parsers: ['sumatra'],
    astFormat: 'estree',
  },
];
export const parsers = {
  sumatra: parseWithSumatraScript,
};

// no printer needed!

@duailibe
Copy link
Member

duailibe commented Dec 20, 2017

Then we can support a generic --parser.javascript flow option for the current --parser flow use case.

I agree with this, but I think JS/TS/Flow can live under a single language-javascript, and then we don't need to complicate that much, can just use language-javascript/printer.js for printing.

@azz
Copy link
Member

azz commented Dec 24, 2017

Setup two new repos successfully using this API 💥

@azz
Copy link
Member

azz commented Dec 24, 2017

Thoughts on using scoped vs non-scoped packages (@prettier/python vs. prettier-python)?

@j-f1
Copy link
Member Author

j-f1 commented Dec 24, 2017

Thoughts on using scoped vs non-scoped packages (@prettier/python vs. prettier-python)?

I think the scoped packages should be used for official (in the @prettier org) plugins, whereas community members would be able to use the prettier-* package names. This would make it clear which ones are officially supported by the team.

@azz
Copy link
Member

azz commented Dec 24, 2017

Sounds good. Looks like @prettier is taken, but I filed a request with npm support to ask if it can be released.

@azz azz closed this as completed in #3536 Dec 26, 2017
@zimme
Copy link
Member

zimme commented Dec 29, 2017

@azz I've added you to the @prettier npm scope. I registered it, for this reason, I just forgot to add you.

@azz
Copy link
Member

azz commented Dec 29, 2017

Awesome! Published the two plugins: prettier/plugin-php#9 & prettier/plugin-python#16.

@azz
Copy link
Member

azz commented Dec 29, 2017

Should we be renaming the repositories?

  • prettier/prettier-python ➡️ prettier/python
  • prettier/prettier-php ➡️ prettier/php

@MichaelDeBoey
Copy link
Contributor

I think all the other packages should go under the org too, but not as a scoped one 🙂

@taion
Copy link
Contributor

taion commented Dec 29, 2017

It's one less step for people cloning the repo if it's prettier/prettier-python instead of prettier/python. I don't think people will want their local checkout to live in a directory named python.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:infra Issues about CI, publishing to npm, or similar type:meta Issues about the status of Prettier, or anything else about Prettier itself
Projects
None yet
Development

No branches or pull requests

7 participants