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

feat: add option for quote and indentation styles #19

Merged

Conversation

manuel3108
Copy link
Member

Relevant for sveltejs/cli#380
Potentially also relevant for svelte-ast-print (cc @xeho91)?

Currently, esrap is hardcoded to always use \t for indentation and ' single quotes.

This PR gives outside consumers the flexibility to specify what they want to use, by providing optional options:

const { code, map } = print(ast, {
  sourceMapSource: 'input.js',
  sourceMapContent: fs.readFileSync('input.js', 'utf-8'),
  indent: ' ', // default '\t'
  quotes: 'single' // or 'double', default 'single'
});

Couple of notes:

  • This is not relevant for svelte itself, as it's only used as a code generation tool there. But this allows codemods to keep the users formatting in place in many more places than it's possible now.
  • I extracted the tests for quotes and indentation into single files and did not use the previous testing setup here. This is because it requires passing custom options to the print function. I deemed this way the testing setup would stay more understandable than adding an options.json file to each testing directory.
  • Moved parsing of ast for testing purposes into it's separate file, so that this method can be re-used across multiple different test cases

Now, onto the more controversial part, that I'm not sure if this should be implemented here. I considered adding an auto option for both properties.

  • indent: 'auto'. This could be implemented like this guessIndentString that's already used in multiple places sv / golden-fleece. "Problem" with this is that it requires a string input, but we only have the ast. We could consider exposing this method from esrap for public use though or let the user optionally provide the original source code as part of the options
  • quotes: 'auto' This could be fairly simple to implement inside esrap as only Literal should be affected by this (rough sample implementation below)
// guessQuoteStyle(): rough idea, untested
let singleQuotes = 0;
let doubleQuotes = 0;
walk(ast, null, {
  Literal(node, { next }) {
    switch (node.raw[0]) {
      case "'": singleQuotes++; break;
      case '"': doubleQuotes++; break;
      default: break;
    }
    next();
  },
});

return singleQuotes >= doubleQuotes ? "'" : '"';

As explained above, I'm uncertain if and how the auto options should be part of esrap, thus I have not implemented them at this point, but i would love your input on this.

@xeho91
Copy link

xeho91 commented Dec 31, 2024

Potentially also relevant for svelte-ast-print (cc @xeho91)?

Absolutely yes! 🧡 In fact, this was on my mind (included in the roadmap), that from v1 I could start working on providing more formatting options, so there's no need to use Prettier or Biome (once it supports Svelte). Currently, I only have indentation because it was the simpliest one.

I could definitely reuse the options interface (and maybe some internal functions) passed down to esrap, to ensure both packages are in sync. I also wish one day svelte-ast-print would be just a plugin to esrap to support Svelte AST nodes (Simon had this on his mind some time ago - reference).

@manuel3108
Copy link
Member Author

I could definitely reuse the options interface (and maybe some internal functions) passed down to esrap, to ensure both packages are in sync. I also wish one day svelte-ast-print would be just a plugin to esrap to support Svelte AST nodes (Simon had this on his mind some time ago - reference).

That would definitely be feasible inside esrap, although I did not look at svelte-ast-print but since you are the author im assuming you are saying this is possible.

Am I understanding you correctly, that you are suggesting that we are publicly exposing those methods, so that they can be reused externally?

Copy link

changeset-bot bot commented Jan 9, 2025

🦋 Changeset detected

Latest commit: d97e56f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
esrap Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris Rich-Harris merged commit 1b1651d into sveltejs:main Jan 9, 2025
6 checks passed
@Rich-Harris
Copy link
Member

thanks!

@Rich-Harris Rich-Harris mentioned this pull request Jan 9, 2025
@manuel3108 manuel3108 deleted the feat/indentation-and-quote-options branch January 9, 2025 21:07
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.

3 participants