-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Build: Load .min.js files even in dev mode, output unminified assets only in prod #32621
Conversation
Size Change: 0 B Total Size: 1.04 MB ℹ️ View Unchanged
|
Can you still load unminified version of bundles shipped with the plugin when consuming from the zip file? |
Could you elaborate on the use-case? The production build will include both an unminified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the main goals of #23960:
and implemented in #31732 was:
This would remove this integration. |
@gziolo That's something that this PR reverts back to the state before #31732, and something that could be further improved. When consuming the plugin from the (official) ZIP file, and running WordPress with After #31732, But that was a regression for the To summarize, this patch reverts one minor change that #31732 introduced, keeps all the other desirable improvements, and avoids reverting the entire #31732. Let me know if that looks OK to you. |
Well that means that the goals of #23960 are not fully achieved yet. We generate and ship readable JS sources with the plugin, but the I assumed that @fullofcaffeine's primary motivation was to solve issues with extracting translations. That's solved and continues to be solved. Another piece of context is that @talldan wants to revert #31732 because it breaks |
@jsnajdr, I'm fine with whatever approach you take. I didn't mean to block this PR. I wanted to clarify where we are now, what this patch changes and if we need to work on a follow-up. It looks like I was correct about the missing integration with |
OK, I'll merge it then. The |
Exactly, sorry if I wasn't clear, and thanks for clarifying, too @jsnajdr @gziolo.
Sounds good, I can take a look soon(ish). I think the next step is to make the |
I thought it was clear, too, because it was the context of all our discussions around this, but then I looked at the PR description and the linked issue and it was all about
On dev build, we don't need the plugin at all. It's useful only for creating production assets that are going to be shipped in the ZIP package to users. But yes, for the production build, the next step is to make the plugin output sourcemaps, too, and to also include them in the published ZIP package. |
Attempt to fix broken sourcemaps in
npm run dev
reported by @talldan in #32617. Got broken in #31732.What's going on? We're running a combination of
npm run wp-env start
(starts a WordPress instance in Docker) andnpm run dev
(compiles the Gutenberg plugin that runs in that instance).The Docker WordPress runs with
SCRIPT_DEBUG=1
and after #31732, that loads theindex.js
unminified asset instead of the minifiedindex.min.js
one. But the unminifiedindex.js
doesn't have a sourcemap, so Devtools can't find it.I fixed this by removing the
SCRIPT_DEBUG
condition fromlib/client-assets.php
and always loadingindex.min.js
. In dev mode (thatnpm run dev
uses) that file is unminified anyway, despite the suffix. Thatindex.min.js
now has a valid sourcemap, so we can debug individual module files instead of the webpack chunk that bundles them all together.I also added a condition to output the unminified
index.js
files only in production build. We don't need them during development and their presence only creates confusion.How to test:
Before this patch, browser loaded
index.js
files and didn't detect any sourcemaps for them.After this patch, browser loads
index.min.js
files, shows the "Source map detected" notice, and the individual module files are part of the source tree.