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

Allow working with multiple entrypoints #58

Merged
merged 11 commits into from
Jul 5, 2024

Conversation

adeys
Copy link
Contributor

@adeys adeys commented Jun 3, 2024

This PR proposes a fix for issues #45 and #46, addressing the need to pass multiple entrypoints as an array to the tailwind builder.

symfonycasts_tailwind:
    input_css:
        - '%kernel.project_dir%/assets/styles/index.css'
        - '%kernel.project_dir%/assets/styles/custom.css'

With this enhancement, users can now specify multiple entrypoints by providing an array of file paths. When building, the entrypoint name should be passed to the command. If omitted, it defaults to the first entrypoint specified in the configuration.

This change enables the following behavior:

  • bin/console tailwind:build assets/styles/custom.css will build the provided CSS file.
  • bin/console tailwind:build will build assets/styles/index.css.

Additionally, the default tailwind.built.css file is no longer generated. Instead, each entrypoint is compiled to a file named based on the original entrypoint name. For example, index.built.css for index.css and custom.built.css for custom.css.

For BC compatibility, the input_css still accepts a string value and requires no changes from the developer

Feedback and improvement ideas are welcomed. Thank you!

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Thank you for creating this PR! I have some questions/comments that I think are important

doc/index.rst Outdated Show resolved Hide resolved
@@ -32,6 +33,7 @@ public function __construct(
protected function configure(): void
{
$this
->addArgument('input', InputArgument::OPTIONAL, 'The input CSS file to compile')
Copy link
Member

Choose a reason for hiding this comment

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

Should we really care about specifying the input file on running the command? Why can't we just build both files (I mean, all the input CSS files listed in the input_css option)? It will simplify things, no need to remember to pass the argument. Because as I understand, if you miss this arg (which is optional) - only the first entry on the input_css will be built. I'm afraid it will lead to WTF moments. IMO we should build all input CSS files listed by default, though I'm not against of keeping that optional arg in case you want to build a specific input CSS file only.

And how about to be consistent and call this input_css to match the config option? Does it make sense to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to build both files, but since the Tailwind binary doesn't natively support this, we will need a workaround:

  • In default mode (without the --watch flag), it would run the process sequentially for each input file (or possibly in parallel if feasible).
  • In watch mode (with the --watch flag), we would need to spawn as many processes as there are input files. I'm not sure if this will be efficient in terms of memory usage. I'll look into potential improvements.

I also think we can keep input_css as the argument name. I initially used input because in the first implementation, I wanted to provide it as a flag. I'll rename it back to input_css.

Copy link
Member

Choose a reason for hiding this comment

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

I see, then the current implementation should be good IMO to keep things simple. But if anyone have any ideas how to improve it even further - feel free to share

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks for your feedback.
I'll maybe try to improve this in another PR.

src/DependencyInjection/TailwindExtension.php Show resolved Hide resolved
src/TailwindBuilder.php Outdated Show resolved Hide resolved
src/AssetMapper/TailwindCssAssetCompiler.php Show resolved Hide resolved
Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I will wait a bit, maybe someone else will leave feedback here

@barbieswimcrew
Copy link
Contributor

This looks good to me.

I will wait a bit, maybe someone else will leave feedback here

I would welcome it if this would finally being merged 🥇

@bocharsky-bw
Copy link
Member

@adeys Thank you for working on it!

@bocharsky-bw bocharsky-bw merged commit 4cde58e into SymfonyCasts:main Jul 5, 2024
11 checks passed
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