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

Scripts: Builds should include source maps by default #45251

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

kkmuffme
Copy link
Contributor

What?

gutenberg builds should include source maps by default

Why?

There is no reason production builds should not include sourcemaps - without sourcemaps, debugging the error or - for users - reporting errors is not possible, making the website and the web in general less accessible

How?

Making the WP_DEVTOOL not conditional on isProduction
Sourcemaps can still be disabled, by explicitly setting WP_DEVTOOL to none

Testing Instructions

npm run build to create the bundled js, which will now include a .map file

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

I agree with this change, but we should get more approvals and a rebase before merging

There is no reason production builds should not include sourcemaps by default - without sourcemaps, debugging the error or - for users - reporting errors is not possible, making the website and the web in general less accessible

* analogous to WordPress#33718
* Fix: WordPress#44278
* Fix: WordPress#41551
@kkmuffme kkmuffme force-pushed the gutenberg-scripts-build-must-include-source-map branch from ee8dff4 to dc8572a Compare December 29, 2022 08:21
@kkmuffme
Copy link
Contributor Author

Rebased

@gziolo
Copy link
Member

gziolo commented Jan 30, 2023

@noahtallen and @kkmuffme, there is also #46812 opened that takes a slightly different approach. As far as I understand, the other one keeps the old behavior by default but lets folks customize the setting when necessary, like in this PR. The alternative seems like a safer bet, but I would like to hear from you both what do you prefer.

@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Jan 30, 2023
@kkmuffme
Copy link
Contributor Author

@gziolo we discussed & concluded somewhere (it's been too long, so I'd have to search it) that it's better to enable sourcemaps by default, since they make the web more accessible and are required so end users can report errors too
Additionally, there is no harm/downside to having sourcemaps by default, so I think changing the default behavior to include sourcemaps (= this PR) is the right way to go.

@gziolo gziolo changed the title gutenberg builds should include source maps by default Scripts: Builds should include source maps by default Jan 30, 2023
@noahtallen
Copy link
Member

I tend to agree with @kkmuffme, but I do think this would be a breaking change for @wordpress/scripts, so I didn't want to merge that without more feedback :)

@gziolo
Copy link
Member

gziolo commented Feb 1, 2023

I tend to agree with @kkmuffme, but I do think this would be a breaking change for @wordpress/scripts, so I didn't want to merge that without more feedback :)

#46812 definitely feels like a safer bet. Here, we need to assess for some scenarios:

  • people don't include source maps from WordPress plugins and themes, but they are listed in the outputted source files. Technically, it doesn't break anything as far as I can tell.
  • creating source maps slows down the build, but in majority of cases it shouldn't be a concern
  • how can people opt out of creating source maps?

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Feb 1, 2023

people don't include source maps from WordPress plugins and themes, but they are listed in the outputted source files. Technically, it doesn't break anything as far as I can tell.

Can you clarify what you mean, bc I am not sure I understand it.

creating source maps slows down the build, but in majority of cases it shouldn't be a concern

Since there is not a single benchmark to be found about how much sourcemaps slow down a build, this isn't a concern for anyone.

how can people opt out of creating source maps?

They can't.
The only reason to not have sourcemaps at all is if you want to "hide"/obfuscate your code, which in my opinion isn't something we should be supporting in the WP ecosystem, as it goes against the core values of WordPress. (and if you really want to do that, you can delete them anyways)
Otherwise, at some point, we will end up with obfuscated PHP too (which is common in the Magento ecosystem for example)

This already is an issue now, if you look at some plugins on woocommerce.com, where the source files + maps are missing (e.g. .coffee, .ts,...) but you get them on request (= makes WP less accessible). An even bigger issue though are popular plugins that do this, where the devs do not provide the source files (and/or maps) at all, e.g. wpml,... These become non-extendable and, should they not be developed anymore, not usable/updateable anymore.

In any way though, you can use DEVTOOL with value hidden-source-map if, for some reason, this is a concern for you (but still get the stacks from the map)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@wordpress/scripts: sourcemap missing in builds
4 participants