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: Add support for format-js script #13394

Closed
wants to merge 1 commit into from
Closed

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jan 21, 2019

Description

Inspired by multiple chats with @zgordon based on the feedback he gets from his online courses.

This PR adds the formatting command for JavaScript code. It uses Prettier behind the scenes to format code. In fact, it uses prettier-eslint-cli which is a combination of prettier and eslint --fix calls to ensure that formatted code stays as close as possible to WordPress JavaScript coding standards. It is still experimental and in many cases will not work as expected.

I would be happy to see also a similar formatter for styles (*.scss).

There is no plan to have these commands included in Gutenberg's workflow.

How has this been tested?

./node_modules/.bin/wp-scripts format-js "**/*.js"

At the moment it doesn't fully work as expected on Gutenberg codebase. On the first run it fixes most of the issues. However, for some reasons, it updates again a few files on every subsequent run which isn't what I would expect. There might be some need to tweak Prettier's default config to make it work (e.g. no-mixed-spaces-and-tabs is very surprising to see). It's also possible that the end goal is not easy to achieve without tweaking the code manually.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling [Package] Scripts /packages/scripts labels Jan 21, 2019
@gziolo gziolo self-assigned this Jan 21, 2019
@nerrad
Copy link
Contributor

nerrad commented Jan 21, 2019

There is no plan to have these commands included in Gutenberg's workflow.

Should we be adding something to an official WordPress package that is not going to be used in WordPress?

@youknowriad
Copy link
Contributor

I'd be up for using it in Gutenberg if we reach an agreement ;)

@gziolo
Copy link
Member Author

gziolo commented Jan 21, 2019

There is no plan to have these commands included in Gutenberg's workflow.

Should we be adding something to an official WordPress package that is not going to be used in WordPress?

There are projects around WordPress core which would benefit from having such command, e.g.:

My reasoning is that we should make it easier to share the same tooling for all projects and this would help with migration.

@gziolo gziolo requested a review from aduth January 21, 2019 14:19
@ntwb
Copy link
Member

ntwb commented Jan 22, 2019

I'm not overly against this, though the bugs I discovered in Prettier when testing the addition of Prettier in #2819 is cause for concern here still IMHO.

I'd certainly like some time to test this before it is merged/shipped

Fingers crossed, I can do that this week, just not today, off to a concert in 10 mins #Mumford&Sons

@gziolo
Copy link
Member Author

gziolo commented Jan 22, 2019

At the moment it doesn't fully work as expected on Gutenberg codebase. On the first run it fixes most of the issues. However, for some reasons, it updates again a few files on every subsequent run which isn't what I would expect. There might be some need to tweak Prettier's default config to make it work (e.g. no-mixed-spaces-and-tabs is very surprising to see). It's also possible that the end goal is not easy to achieve without tweaking the code manually.

@ntwb as mentioned in the description and highlighted above, I have mutual feelings. It isn't perfect and needs more work.

@ajitbohra
Copy link
Member

loving the way wp-scripts is evolving ❤️

@@ -1,3 +1,4 @@
module.exports = {
root: true,
Copy link
Member

Choose a reason for hiding this comment

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

Is this a bug worth fixing separately, lest it be stalled otherwise by the progress of the other changes in the pull request?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I'm not sure if it is a bug. It might be. I figured out it's safer to put it here :)
I will open another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

#13483 is ready

@gziolo gziolo added the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Feb 22, 2019
@gziolo
Copy link
Member Author

gziolo commented Mar 19, 2019

I'm closing it for now as it isn't solid enough and I don't have time to investigate it further.

@gziolo gziolo closed this Mar 19, 2019
@gziolo gziolo deleted the add/scripts-format-js branch March 19, 2019 09:03
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants