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

Script debug when version controlled #12457

Closed
wants to merge 1 commit into from
Closed

Conversation

ellatrix
Copy link
Member

Description

When I setup Gutenberg for local development, I'd expect it to load unminified files.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ellatrix ellatrix requested a review from a team November 30, 2018 10:26
function gutenberg_script_debug() {
return (
( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ) ||
file_exists( gutenberg_dir_path() . '.git' )
Copy link
Contributor

Choose a reason for hiding this comment

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

What if, in the course of development, I do want to test with minified souces? Switching on the existence of .git seems like a very inflexible method.

Copy link
Member Author

Choose a reason for hiding this comment

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

The same is true if we set it in the Docker config: you'd need to edit the config.

Copy link
Member

Choose a reason for hiding this comment

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

An environment variable seems the best option here; why not have MINIFY_JS, SCRIPT_DEBUG, or something else, be an env var we could check for?

Copy link
Member

@tofumatt tofumatt 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 @mcsf that checking on .git is a bit too inflexible, but using an env var would work well here I think.

function gutenberg_script_debug() {
return (
( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ) ||
file_exists( gutenberg_dir_path() . '.git' )
Copy link
Member

Choose a reason for hiding this comment

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

An environment variable seems the best option here; why not have MINIFY_JS, SCRIPT_DEBUG, or something else, be an env var we could check for?

@gziolo gziolo added the [Type] Build Tooling Issues or PRs related to build tooling label Jan 31, 2019
@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 8, 2019
@gziolo
Copy link
Member

gziolo commented Mar 8, 2019

Is there any way to move it forward? This PR got out of date but it would be awesome to find a way to enable this flag through Docker.

@pento
Copy link
Member

pento commented Mar 9, 2019

You can do it by adding these lines to install-wordpress.sh:

docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm $CLI config set WP_DEBUG true --raw --type=constant
docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm $CLI config set SCRIPT_DEBUG true --raw --type=constant
docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm $CLI config set WP_DEBUG_DISPLAY true --raw --type=constant

Location in the file doesn't matter, they just need wp-config.php to exist, which the WordPress container automatically creates.

@gziolo
Copy link
Member

gziolo commented Mar 11, 2019

@pento this is something @aduth figured out in one of the recent PRs working on something unrelated. I will close this PR and open another one which offers ENV variable to to flip those constant from CLI.

@gziolo gziolo closed this Mar 11, 2019
@gziolo gziolo deleted the try/script-debug branch March 11, 2019 08:38
@gziolo
Copy link
Member

gziolo commented Mar 11, 2019

I opened #14371 where I want to provide an option to override site constants through env variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants