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

Rename tailwind integration options #7391

Merged
merged 4 commits into from
Jun 15, 2023
Merged

Rename tailwind integration options #7391

merged 4 commits into from
Jun 15, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jun 14, 2023

Changes

Following up #6724 (major), this renames:

  • config.path to configFile
  • config.applyBaseStyles to applyBaseStyles

The config. prefix is a bit redundant 🤔 I also chose configFile over configPath as that's the same name used by Vite and vite-plugin-svelte.

Testing

Existing test should pass

Docs

Updated the README to reflect the new option names.
/cc @withastro/maintainers-docs for feedback!

@bluwy bluwy requested a review from a team as a code owner June 14, 2023 10:04
@changeset-bot
Copy link

changeset-bot bot commented Jun 14, 2023

🦋 Changeset detected

Latest commit: c893d87

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

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) labels Jun 14, 2023
@ematipico
Copy link
Member

ematipico commented Jun 14, 2023

I also chose configFile over configPath as that's the same name used by Vite and vite-plugin-svelte.

Note: Disclaimer: I don't want to bikeshed.

Usually, the terminology "path" is used for types/structures that don't necessarily exist on file system and they do not necessarily belong to a physical file. Instead, the terminology "file" is used for those types/structures that read/write to file system, which means they must be derived/computed from "path" that exists.

This means that when I read configFile, I assume I can read its contents. Is it the case?

@bluwy
Copy link
Member Author

bluwy commented Jun 14, 2023

Yes! configFile should always be a file path that exist in the filesystem. This value is passed to tailwind, which IIRC the internal code simply does fs.readFile(path.resolve(configFile), 'utf-8').

@ematipico
Copy link
Member

That's what got me! It's a path, we don't pass the actual contents 😅

@bluwy
Copy link
Member Author

bluwy commented Jun 14, 2023

I'm confused if you're proposing either configFile or configPath here 😅 But I think I'm leaning towards the current one for now for consistency.

@delucis
Copy link
Member

delucis commented Jun 14, 2023

Are we documenting migration steps somewhere if this is a major release with breaking changes? I think the changelog for the major release should include clear descriptions highlighting:

  • What broke?
  • Under which circumstances?
  • What do you need to do to your existing project to upgrade?

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I would personally use configPath, but it's just a personal preference. No need to block it

@bluwy
Copy link
Member Author

bluwy commented Jun 14, 2023

@delucis I'm currently documenting them in the changesets. There's two migration needed:

  1. option name change (this PR)
  2. tailwind config loading behaviour change (@astrojs/tailwind: simplify, upgrade & fix support for ts config file #6724)

In this PR, I've tweaked the changeset for no2 to reflect the new option name (real-spies-pretend.md). I suppose they don't currently answer your 3 points well that can be improved 😅

EDIT: improved!

@bluwy bluwy mentioned this pull request Jun 14, 2023
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great so far, @bluwy! I left some suggestions below, and as Chris said, for a breaking change we'll really need in this document to show the diff example you have in the changeset.

With that section, we can do the kinds of things I've suggested below. See what you think!

.changeset/real-spies-pretend.md Outdated Show resolved Hide resolved
.changeset/real-spies-pretend.md Show resolved Hide resolved
packages/integrations/tailwind/README.md Show resolved Hide resolved
packages/integrations/tailwind/README.md Show resolved Hide resolved
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @bluwy! I think these look good now!

@bluwy bluwy merged commit 556fd69 into main Jun 15, 2023
@bluwy bluwy deleted the tailwind-options branch June 15, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants