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

Documentation: Add tutorial explaining how to use the autogenerated asset file when using wp-scripts #17423

Closed
jrchamp opened this issue Sep 12, 2019 · 9 comments · Fixed by #17489
Assignees
Labels
[Feature] Block API API that allows to express the block paradigm. Good First Issue An issue that's suitable for someone looking to contribute for the first time [Status] In Progress Tracking issues with work in progress [Tool] WP Scripts /packages/scripts [Type] Developer Documentation Documentation for developers

Comments

@jrchamp
Copy link
Contributor

jrchamp commented Sep 12, 2019

Describe the bug
PR #15124 introduced automated script dependency files as .deps.json files. These should allow you to dynamically rebuild the dependencies lists that get passed to wp_register_script(). Looking at the examples in the Block Tutorial guide, I would expect that the ESNext files would generate dependency files that align with the set of dependencies in the matching function examples ( such as https://developer.wordpress.org/block-editor/tutorials/block-tutorial/creating-dynamic-blocks/ ).

Instead, you just get the default values:

["wp-element","wp-polyfill"]

Based on my testing, this appears to be because the ESNext examples use the wp.blocks and wp.data global objects (instead of an import), which makes sense if those global objects are always available. However, if those global objects are only available because an existing script listed them as a dependency, then those global objects are indeed dependencies that should make it into the .deps.json file. If, perhaps, the recommendation is to use import instead of the globally available objects for automated dependency resolution, then the Block Tutorial guide should be updated to match such a recommendation.

Overall, this seems like either a documentation issue or an automatic dependency resolution issue, but that will depend on what you believe the best practice to be.

Thank you for all the work that you do, especially the documentation.

To reproduce
Steps to reproduce the behavior:

  1. Go to Block Tutorial
  2. Copy ESNext example into src directory
  3. npm build
  4. See that .deps.json file doesn't contain all dependencies listed in Block Tutorial

Expected behavior
Documentation that matches best practice recommendations and tooling results. Tooling that works with documentation examples.

Additional context

  • @wordpress/scripts version 4.1.0
@gziolo
Copy link
Member

gziolo commented Sep 16, 2019

Documentation that matches best practice recommendations and tooling results. Tooling that works with documentation examples.

I think the easiest way to move forward is to start using import statements in ESNext examples rather than wp globals to show the full power of wp-scripts :)

I have no idea how much work would it require to teach Webpack to recognize wp globals as a way to import dependencies, but it seems like it might create some confusion. The idea is that all the additional features wp-scripts build brings should help you teach how to opt-in for all ESNext features and import statements are one of them.

@nerrad and @sirreal - do you have some other thoughts to share on this topic?

@nerrad
Copy link
Contributor

nerrad commented Sep 17, 2019

I think the easiest way to move forward is to start using import statements in ESNext examples rather than wp globals to show the full power of wp-scripts :)

I'm on the fence. The doc examples will work in a build system with or without wp-scripts (in a WordPress 5.0+ environment) because the globals are present. So that leads to less "gotchas" when it comes to people following the docs without using wp-scripts as a base.

I think the path forward here to make sure it's clear in the documentation for the dependency extraction webpack plugin that it only extracts dependencies explicitly imported (not implicitly using the global). Since it's also included in wp-scripts automatically, it'd probably be a good idea to make it clear in that package as well?

@gziolo gziolo added Needs Dev Ready for, and needs developer efforts Good First Issue An issue that's suitable for someone looking to contribute for the first time [Feature] Block API API that allows to express the block paradigm. labels Sep 19, 2019
@gziolo
Copy link
Member

gziolo commented Sep 19, 2019

I think the path forward here to make sure it's clear in the documentation for the dependency extraction webpack plugin that it only extracts dependencies explicitly imported (not implicitly using the global). Since it's also included in wp-scripts automatically, it'd probably be a good idea to make it clear in that package as well?

We can add this note to make it clear. However, I doubt it will be easy to discover for those using wp-scripts because there is a level of indirection. I think we need to add a new page to the tutorials which shows how to refactor the existing code from wp globals to imports allowing to take advantage of the asset file. We can alternatively extend what's included in https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/tutorials/javascript/js-build-setup.md.

/cc @mkaz @nosolosw

@gziolo gziolo changed the title Suggested Dependencies and Automatic Dependencies Mismatch Documentation: Add tutorial explaining how to use the autogenerated asset file when using wp-scripts Sep 19, 2019
@gziolo gziolo removed the Needs Dev Ready for, and needs developer efforts label Sep 19, 2019
@mkaz
Copy link
Member

mkaz commented Sep 19, 2019

I think extending the existing docs make the most sense. I have a set of materials developed for internal training that I plan to convert for public release. Just needs some buttoning up and refinement, but includes this exact example around dependencies.

I'll try to get a PR ready either this week or earlier next.

@mkaz mkaz self-assigned this Sep 19, 2019
@gziolo
Copy link
Member

gziolo commented Sep 19, 2019

I think extending the existing docs make the most sense. I have a set of materials developed for internal training that I plan to convert for public release. Just needs some buttoning up and refinement, but includes this exact example around dependencies.

I'll try to get a PR ready either this week or earlier next.

Sounds great, thank you for all the great work with tutorials 💯

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Sep 19, 2019
@sirreal
Copy link
Member

sirreal commented Sep 23, 2019

do you have some other thoughts to share on this topic?

I tend to agree with what's been said. I'd encourage use of import which makes dependencies explicit. We have a webpack plugin that seems to be working well. At the implementation level, we can analyze the module requests to extract the dependency list.

If we were to extract dependencies from expressions like wp.data.select( 'core' )…, I believe we'd need to implement Webpack parser hooks and handle much more complex analysis.

Encouraging and transitioning to explicit import seems like the best path forward.

@gziolo
Copy link
Member

gziolo commented Sep 23, 2019

If we were to extract dependencies from expressions like wp.data.select( 'core' )…, I believe we'd need to implement Webpack parser hooks and handle much more complex analysis.

Encouraging and transitioning to explicit import seems like the best path forward.

The easiest way to solve it is to expose the name of the store from the module and import it explicitly:

// in @wordpress/core-data
export const STORE_NAME = 'core';

// in other places
import { STORE_NAME } from '@wordpress/core-data';

const something = wp.data.select( STORE_NAME )....

@sirreal
Copy link
Member

sirreal commented Sep 23, 2019

My example of wp.data.select( 'core' ) was intended an example of an expression we'd need to parse in order to build the dependency list, it could have been wp.apiFetch( … ), wp.url.getQueryArg( … ), etc.

What I want to highlight is that import apiFetch from '@wordpress/api-fetch'; is unambiguous and can be easily extracted to the dependency list.

@gziolo
Copy link
Member

gziolo commented Sep 23, 2019

Yes, I get that but this is also an actual issue which this kind of automation uncovers. That said I totally agree that using import statements is the most straightforward way to go. If you think about it more, all the ways you could reverse engineer wp globals to convert them into a proper list of dependencies will never be solid enough as there are multiple ways of encoding it and composing:

wp.data;
wp[ 'data' ];
const x = wp; x.data;
const y = 'data'; wp[ y ];
// ...and so on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. Good First Issue An issue that's suitable for someone looking to contribute for the first time [Status] In Progress Tracking issues with work in progress [Tool] WP Scripts /packages/scripts [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants