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

Bundle Set up #1

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Bundle Set up #1

merged 1 commit into from
Aug 18, 2023

Conversation

WebMamba
Copy link
Contributor

No description provided.

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.

Some minor text tweaks :)

Copy link
Contributor

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

TYVM for getting this going! I dropped some comments

README.md Outdated
{# templates.base.html.twig #}

{% block stylesheets %}
<link rel="stylesheet" href="{{ asset('styles/app.css') }}">
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a proposed change. I also wonder what @bocharsky-bw thinks about this.

Right now, the bundle works by reading assets/styles/app.scss and outputting assets/styles/app.css. You should then ignore this file from git. We can get fancier. Instead:

A) The user would reference the app.scss file directly - href="{{ asset('styles/app.scss') }}"
B) Like with TailwindBundle, we would output the Sass process to an internal, temporary file - e.g. var/sass/app.built.css
C) Like with TailwindBundle, we would "swap" the contents of the styles/app.scss file in AssetMapper for the contents of this var/sass/app.built.css file.

The only extra step that this bundle would need that TailwindBundle did not need is:

D) "Remap" the output filename of styles/app.scss in AssetMapper. If you do A-C, you will end up with a final file called something like public/assets/styles/app.12345abcdef.scss - ending in .scss, which we do not want. By decorating the core PublicAssetsPathResolverInterface, we can change the output name of this file to end in .css.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, on the one hand, linking to a CSS file is a more straightforward way... but as you said that file should be probably "gitignored", so I think it really better to link directly to the Scss file then excluding working with auto-generated file directly. And I like to make the behavior consistent with TailwindBundle 👍

@WebMamba
Copy link
Contributor Author

Thanks for your review @weaverryan and @bocharsky-bw. I fix the syntax issues you point out, rename webmamba to symfonycasts, and now the user reference directly app.scss as in the last comment. Tell me what you thinks about all this changes! Cheers 😁🧡

Copy link
Contributor

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Ran out of time to fully review - but I have some to keep you moving! More coming hopefully tomorrow. Looking good - excited to get this done.

{
$path = $this->decorator->resolvePublicPath($logicalPath);

if (str_contains($path, '.scss')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we would target JUST the one .scss file. Since we are only given the logical path here... and the root_sass config is an absolute path, that makes it tricky.

WDYT about changing the root_sass to be the asset mapper "logical path" instead? Then it would be easy to compare that logical path to this logical path. And, it makes sense: your "input Sass" file MUST be in an AssetMapper path for all of this to work. If we decide to make that change and it works, I'll do the same for tailwind-bundle.

Copy link
Contributor Author

@WebMamba WebMamba Jul 24, 2023

Choose a reason for hiding this comment

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

Hum sorry I don't get what you mean here. Maybe something I miss understands with asset mapper

Copy link
Contributor

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

A few more comments - now fully reviewed. Thanks :)

}
],
"require": {
"php": ">=8.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

was there ultimately an extension needed for unzipping the tar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it's enable by default: https://www.php.net/manual/en/phar.installation.php

@@ -0,0 +1,3 @@
p {
color: red;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be here? I'm asking because FunctionalTest seems to create this in its setUp().

Also, if we set a variable then use it, the assertion in the test will be a bit more meaningful, as we'll see Sass doing its job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is for the SassBuilderTest

@bendavies
Copy link

bendavies commented Jul 20, 2023

I think i have a feature request to be considered before this is merged.

The request is to support compiling/watching more than one scss file.
This is required to support page specific css, and to migrate effectively from "multiple entrypoints" in webpack encore.

this is supported fine by the cli:
https://sass-lang.com/documentation/cli/dart-sass/#many-to-many-mode

this would require changing the root_sass configuration option to an array node to accept multiple paths

@weaverryan
Copy link
Contributor

That makes sense to me. I can’t immediately think why this would cause any major issues.

@WebMamba WebMamba requested review from weaverryan and bendavies July 24, 2023 16:28
@WebMamba
Copy link
Contributor Author

@bendavies thanks for your review! I implemented the support for multiples sass files. 😁

{
$fileName = basename($sassFile, '.scss');

return $outputDirectory . '/' . $fileName . '.output.css';
Copy link

@bendavies bendavies Jul 24, 2023

Choose a reason for hiding this comment

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

i think this should just be $filename.css so it aligns with the assetmapper docs nicely. also phpstorm has nice integration if the filenames match like this

@bendavies
Copy link

@bendavies thanks for your review! I implemented the support for multiples sass files. 😁

great! thanks

Copy link
Contributor

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Left some comments - but this is working really nicely now! After these comments are addressed, we should be able to merge.

Things still needed after that:

A) Adding CI
B) #2 (not a blocker for an initial release, but this would be REALLY good to have)
C) Thinking about a shared "build" bundle that Tailwind & Sass could both use - the idea being that someone could, in theory, use Tailwind AND Sass and use some central assets:build command that would take the source contents through a small pipeline (e.g. run Sass on a file, take its output, run Tailwind on that to get the final contents).

unlink($targetPath);
unlink($this->binaryDownloadDir.'/'.self::getBinaryName().'.tar');

chmod($this->binaryPath, 0777);
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails because, by default, $this->binaryPath is null. This should actually reuse the logic from createProcess() where it uses $this->binaryDownloadDir.'/dart-sass/sass';.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should move the binary into var/sass instead of bin/... as there is no reason for the user to wonder whether they should commit this file or not. This is how it's done in TailwindBundle - https://github.com/SymfonyCasts/tailwind-bundle/blob/main/config/services.php#L20C17-L20C22 - that's eventually passed to new TailwindBinary(): https://github.com/SymfonyCasts/tailwind-bundle/blob/main/src/TailwindBuilder.php#L101C42-L101C56

Copy link

@bendavies bendavies Aug 9, 2023

Choose a reason for hiding this comment

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

I'm not sure how I feel about that. This pretty much breaks all existing conventions of having binaries in bin.
How about keeping in bin and adding a recipe to add a git ignore entry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep in sync with the TailwindBundle for now. But I agree with @bendavies 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

The binary is a completely hidden thing - the user would never need to run it directly (and won't commit it). My thoughts: why put it in bin/ for the user to see and worry about?

@WebMamba
Copy link
Contributor Author

WebMamba commented Aug 9, 2023

Great! Lets have a final check, all your comment should be fixed now! 😁

@weaverryan
Copy link
Contributor

This is working fantastically! Thank you a million @WebMamba!

@weaverryan weaverryan merged commit 8954514 into SymfonyCasts:main Aug 18, 2023
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.

None yet

4 participants