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

Add the 'jsCustomTags' option #6626

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MeLlamoPablo
Copy link

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

==========================

Hello!

First of all I want to say that I realize that this PR will be controversial. I have read the opinion philosophy and #40.

This PR adds a new option jsCustomTags with the following form:

{
  "jsCustomTags": {
    "styled-components": ["media"]
  }
}

I believe that this option is necesary and must make it into prettier (or any similar option that cover this use case). I understand that we want to keep prettier as opinionated as possible, but in this case this is not a matter of opinion, this is a matter of supporting an use case.

In our code, we use styled-components, and we have a custom helper to do media queries:

const MyComponent = styled.div`
    max-width: 700px;

    ${media.mobile`
        max-width: 90%;
    `};
`;

However, in another one of our projects, we do:

const MyComponent = styled.div`
    max-width: 90%;

    ${from.mobile`
        max-width: 700px;
    `};
`;

We use from instead of media because our queries are mobile-first, and they are more descriptive that way. We are not the only ones to use this pattern: take a look at this article or this package.

The point here is that we can't just let prettier decide that the only valid tag names for styled-components are styled and css. There's an open issue about supporting keyframes too (#5936), but I believe that opening a PR just for keyframes is missing the point. What we need to move towards is a customizable list of allowed tags, in which styled and css are the default, but can also be extended.

There is no standard way to define media queries in styled-components (neither in styled-components itself nor there is a well established standard in the community - like fsa for redux), and while there isn't, we can't have prettier decide which names are acceptable and which aren't.

This not only applies to styled-components, but to any potential library that relies on tagged template strings like graphql; hence why I named the option jsCustomTags and not styledComponentCustomTags.

Some final notes:

  • I opened this PR knowing that there's a good chance that it won't be merged. I did it instead of opening an issue because I wanted to present a working prototype, and because we can continue to use our own workflow without giving up auto formatting inside media queries.
  • I am aware that some tests broke because of this PR, mostly snapshot of configuration help. I didn't want to bother updating them without knowing if this was going to be merged, and if it is, there's a good chance that the API won't be exaclty what I propose, so the tests can be updated later.

Thanks a lot for your time.

@alexander-akait
Copy link
Member

Thanks.

Just notes:

  • we should allow to define foo.bar.baz.other.name and any deep

What about?

{
  "jsCustomTags": ["styled.div", "styled.keyframes", "from.mobile"]
}
  • default value should handle all currently supported tagged template expressions to avoid breaking change

Bonus: Also it can solve problems when we break code in tagged template expressions, after this PR it should be easy to fix, just setup jsCustomTags to [].

In fact, we should consider whether we should support formatting template literal or not.

Here two solutions:

/cc @prettier/core will be great to get some feedback

@thorn0
Copy link
Member

thorn0 commented Oct 8, 2019

Alternatively, why not try to format all tagged template literals nested to styled.div as CSS?

@MeLlamoPablo
Copy link
Author

What about?

{
   "jsCustomTags": ["styled.div", "styled.keyframes", "from.mobile"]
}

@evilebottnawi, but how would you distinguish between graphql, styled-components and my own custom DSL that I just wrote a plugin for? I think that it sucks having to deal with a nested structure like that but I don't see any other alternative.

Alternatively, why not try to format all tagged template literals nested to styled.div as CSS?

@thorn0, that's an interesting idea indeed, but it would prevent me from doing:

const responsiveStyles = media.mobile`
    max-width: 90%;
`;

const MyComponent = styled.div`
    max-width: 700px;
    ${responsiveStyles};
`;

Personally I think that's confusing so I don't do it, but it's possible so I'm guessing it should be supported too.

@MeLlamoPablo MeLlamoPablo reopened this Oct 8, 2019
@alexander-akait
Copy link
Member

@MeLlamoPablo

but how would you distinguish between graphql, styled-components and my own custom DSL that I just wrote a plugin for? I think that it sucks having to deal with a nested structure like that but I don't see any other alternative.

Maybe support also this:

  "jsCustomTags": ["...", "from.mobile"]

... means use all default tags + own

@thorn0 yep, but here more fundamental problem, in future style-components can implement more helpers and for us it is not easy maintenance all supported tagged template literals

@MeLlamoPablo
Copy link
Author

@evilebottnawi sorry I didn't explain myself very well. My point is that in this example, we want to parse the content from-tagged strings with styled-components syntax:

const MyComponent = styled.div`
    max-width: 90%;

    ${from.mobile`
        max-width: 700px;
    `};
`;

But let's assume that I create a DSL that allows to specify a TCP origin (weird example I know, but let's assume that we need a DSL for that and I write a prettier plugin so it can be properly parsed). We'd have:

import { from } from 'my-custom-dsl';

const remote = from`
    protocol ssh origin example.com port 22
`;

And we'd want:

import { from } from 'my-custom-dsl';

const remote = from`
    protocol ssh
    origin example.com
    port 22
`;

So, how will prettier know if it needs to format from-tagged strings as styled-components or as my-custom-dsl? I'd need to specify either:

{
  "jsCustomTags": {
    "styled-components": ["from"]
  }
}

or

{
  "jsCustomTags": {
    "my-custom-dsl": ["from"]
  }
}

@alexander-akait
Copy link
Member

hm, maybe we should allow to execute multiple plugins for extension (maybe already implemented, need tests) and you can implement an own sub plugin for DSL, also it is allow to us move all library specific code to sub own plugins

@lydell
Copy link
Member

lydell commented Oct 9, 2019

I think it might be worth thinking about #5588 when designing this.

@thorn0 thorn0 added the type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity. label Oct 30, 2019
@JounQin
Copy link
Member

JounQin commented Nov 16, 2019

We should merge some options to be a nested object IMO.

I do have some issues on formatting HTML in ts, we're writing vscode snippets and we do want the snippet to be prettier formatted, but it respects original ts indent from ts what makes the final snippet outputs incorrect indent, what a pity.

@brandonkal
Copy link

I would like to see yaml tagged template support. Prettier already supports both languages standalone, so ideally this PR would be made more generic.

@nickmccurdy
Copy link
Member

Would this functionality be covered by #5588?

@JounQin
Copy link
Member

JounQin commented Jan 8, 2024

This should already be covered by https://github.com/Sec-ant/prettier-plugin-embed?

cc @fisker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants