Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#64933 closed defect (bug) (fixed)

Block library styles are present in two locations

Reported by: desrosj's profile desrosj Owned by:
Milestone: 7.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

Follow up to #64393.

In [61438], all wp-includes/blocks/*/(style|editor|theme)(-rtl).css files were added to each named subdirectory within wp-includes/css/dist/block-library/ (corresponding build server commit).

The files are identical and one of them needs to be removed. It's unclear whether the intention was for these files to remain in each respective wp-includes/blocks/ directory, or to move into css/dist/block-library. I think either one is reasonable.

If the intention was to leave them as is, then copying to css/dist/block-library needs to be stopped.

If the intention was to move them, then the wp-includes/blocks versions need to be removed and the related style registration code needs to be updated (the original location is being used to load block styles).

Change History (14)

This ticket was mentioned in PR #11337 on WordPress/wordpress-develop by @desrosj.


3 months ago
#1

This removes duplicate block library-related CSS files in favor of the same location as WP 6.9: wp-includes/blocks.

Trac ticket: Core-64933

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude
Model(s): Sonnet 4.6
Used for: Creating the initial PR based on a description of the problem.

This ticket was mentioned in PR #11338 on WordPress/wordpress-develop by @desrosj.


3 months ago
#2

This removes duplicate block library-related CSS files in favor of moving all files to wp-includes/css/dist/block-library/*.

Trac ticket: Core-64933

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude
Model(s): Sonnet 4.6
Used for: Creating the initial PR based on a description of the problem.

#3 @desrosj
3 months ago

I've created two pull requests: one that assumes the previous location is the correct one, and one that assumes the css/dist location is the correct one.

Based on what I can see, I think the previous location is the preferred one. Especially since BLOCK_DIR is a defined constant used when handing the styles. But I can't say for certain.

@youknowriad Could you take a look and clarify what the original intention was from the work on #64393?

#4 @jorbin
3 months ago

Based on what I can see, I think the previous location is the preferred one.

I agree with this. It also means we don't need to update $_old_files.

@youknowriad commented on PR #11337:


3 months ago
#5

How are we copying these styles from Gutenberg to Core?

@youknowriad commented on PR #11337:


3 months ago
#6

The intent was not to move them to a new location with the build change, but we still need a way to copy them from the built Gutenberg. where is that being done now?

@youknowriad commented on PR #11337:


3 months ago
#7

I see that some of what this script was meant to do has now moved to the Gruntfile. I'd love to understand more why this was done. I'd rather we move away from Grunt iteratively, and would prefer a single "copy" script for all the files that we move from Gutenberg build to a specific place in Core. I think that's clearer over random and unclear Grunt plugins.

#8 @manhar
3 months ago

I agree, I think this is an unintentional duplication of build files. I think removing the duplicate files from wp-includes/css/dist/block-library and keeping wp-includes/blocks as the canonical source would be the best solution, as the original location is currently used to load block styles.

#9 @desrosj
3 months ago

In 62103:

Build/Test Tools: Only copy block library-related CSS to a single location.

Since [61438], all CSS files for the block library are being copied twice into two separate locations: wp-includes/blocks/BLOCKNAME/ (the previous location) and wp-includes/css/dist/block-library/BLOCKNAME/.

Since the copies added to the css/dist directory are not actually being used and moving the files was the original intention in [61438], this adds an exclusion rule to the grunt copy:gutenberg-styles task to allow the tools/gutenberg/copy.js script to handle these files.

Props jorbin, youknowriad.
See #64933.

@desrosj commented on PR #11338:


3 months ago
#10

Merged in r62103.

#11 follow-up: @desrosj
3 months ago

  • Resolution set to fixed
  • Status changed from new to closed

@manhar Apologies, I missed your name in the props list in r62103. I've gone and manually added props for you to this commit and it should now be showing on your w.org profile.

Confirmed that this is now resolved on the build server. Closing out.

@desrosj commented on PR #11337:


3 months ago
#12

The intent was not to move them to a new location with the build change, but we still need a way to copy them from the built Gutenberg. where is that being done now?

Thanks for confirming! I went ahead and committed #11338 in r62103 to resolve this and confirmed that the build server reflected the desired outcome.

I see that some of what this script was meant to do has now moved to the Gruntfile. I'd love to understand more why this was done. I'd rather we move away from Grunt iteratively, and would prefer a single "copy" script for all the files that we move from Gutenberg build to a specific place in Core. I think that's clearer over random and unclear Grunt plugins.

Unfortunately, the original changes introduced more questions than were answered and created many more problems than were solved. Apologies that the refinements made since r61438 run counter to your personal preference. It was not at all clear that the desired outcome is to move away from Grunt entirely given that the original r61438 changeset introduced 5 new tasks and moved the handling of the wp-includes/js/dist/vendor files into Grunt. r61438 also removed nearly all of the Webpack-related code. Does that mean your goal is also to move away from Webpack entirely?

I'll admit, despite having spent over a month working on refining the build script (it took me over a week just to understand all of the moving parts after the changes made before I was confident enough to open pull requests to suggest improvements), I'm still very unclear about what the various motivations behind that changeset actually are and which parts of that commit address each one. Any time a major change to the underlying tooling is made, there must be clearly stated goals, a plan or roadmap when multiple stages are needed, reasonably broad discussions to ensure as many factors are accounted for as possible, and a level of confidence that there's a consensus that the new tool or approach is worth the tradeoffs. While the PR was lengthy and the ticket has had lots of discussion, I still have not seen this clearly addressed.

As for the rationale behind behind moving things out of custom scripts into Grunt, the overall goals guiding the decisions made since r61438 are:

  • Avoid having multiple tools performing identical actions for different areas of the code base: copying files and directories, removing occurrences of sourceMappingURL= in JavaScript files, adding comments/file headers to files compiled in the build process, etc.
  • Avoiding needless complexity: many wheels were reinvented through custom code without strong and/or compelling reasoning
  • Ensure that the results of the build script are reproducible regardless of the date/time, local file locations/paths, or platform the script is run on.
  • Building/copying/transforming only the intended files to the intended locations: many files were being included in the built package unintentionally, even after many refinements contributing to a package size for 7.0 that was more than double that of 6.9.

Grunt is not perfect by any means. But despite being considered a legacy tool, it still meets the overwhelming majority of the needs for this repository. There have been several discussions in the past to replace Grunt entirely, but those have all targeted Webpack as the replacement (see Core-49423 and Core-43731) and stalled for various reasons. I think this demonstrates that the broader contributing community agrees with you, but a solution that is worth the tradeoffs has not yet risen to the top.

#13 in reply to: ↑ 11 @manhar
3 months ago

Replying to desrosj:

@manhar Apologies, I missed your name in the props list in r62103. I've gone and manually added props for you to this commit and it should now be showing on your w.org profile.

Confirmed that this is now resolved on the build server. Closing out.

@desrosj Thanks for the update. appreciate the help!

@youknowriad commented on PR #11337:


3 months ago
#14

Thanks for explaining.

It's not about Grunt being a legacy tool or not, it's more about clarity of flows.

1- Checkout Gutenberg
2- Build Gutenberg
3- Copy output from Gutenberg to the right places since it's already built.

Grunt is organized in random chunks without clear logic. With the single copy script the purpose of the tool is clear. I realized you merged 1 and 2 using a build step in Gutenberg so right now it's more

1- Checkout and unzip.
2- Copy built files to the right places.

Note: See TracTickets for help on using tickets.