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

Import names aren't validated before writing dependency file #15876

Closed
iandunn opened this issue May 29, 2019 · 7 comments · Fixed by #27387
Closed

Import names aren't validated before writing dependency file #15876

iandunn opened this issue May 29, 2019 · 7 comments · Fixed by #27387
Assignees
Labels
[Status] In Progress Tracking issues with work in progress [Tool] ESLint plugin /packages/eslint-plugin [Tool] WP Scripts /packages/scripts [Type] Enhancement A suggestion for improvement.

Comments

@iandunn
Copy link
Member

iandunn commented May 29, 2019

Describe the bug

dependency-extraction-webpack-plugin doesn't validate import names. If an invalid name is used, the parent script will silently fail to load, which can be confusing and time-consuming to fix.

Expected behavior

The watch script should throw an error to the console, to alert the dev that they used an invalid dependency.

To reproduce

  1. Enqueue a script that depends on a deps.json file. e.g.,
wp_enqueue_script(
	'foo',
	plugins_url( 'dist/foo.js', __FILE__ ),
	json_decode( file_get_contents( __DIR__ . '/dist/foo.deps.json' ) ),
);
  1. Make a type in an import statement. e.g.,
import { Button } from '@wordpress/component'; // missing an `s` at the end
  1. Load the page where the script should be enqueue, notice that it's no longer there.
@iandunn iandunn added [Type] Enhancement A suggestion for improvement. [Tool] WP Scripts /packages/scripts labels May 29, 2019
@gziolo gziolo added the [Tool] Dependency Extraction Webpack Plugin /packages/dependency-extraction-webpack-plugin label Aug 12, 2019
@gziolo
Copy link
Member

gziolo commented Aug 23, 2019

It seems like this should be handled on the linter side instead. Well, at least this is what we introduced in Gutenberg lately with “import/no-extraneous-dependencies” rule in #16969. It’s handled per package and only for non-test files in Gutenberg but it might be a good idea to set it in recommended config of ESLint and override in the project as Gutenberg is a special case here. @nerrad, @ntwb, and @swissspidy what do you think?

@nerrad
Copy link
Contributor

nerrad commented Aug 23, 2019

Ya I agree, linter. A couple IDE's I've used will also warn on invalid imports.

@gziolo gziolo added [Tool] ESLint plugin /packages/eslint-plugin Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts and removed [Tool] Dependency Extraction Webpack Plugin /packages/dependency-extraction-webpack-plugin labels Oct 20, 2019
@gziolo gziolo added [Status] In Progress Tracking issues with work in progress and removed Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts labels Mar 14, 2020
@gziolo gziolo self-assigned this Mar 14, 2020
@gziolo gziolo removed their assignment May 9, 2020
@gziolo gziolo added Needs Dev Ready for, and needs developer efforts and removed [Status] In Progress Tracking issues with work in progress labels May 9, 2020
@gziolo
Copy link
Member

gziolo commented May 9, 2020

My attempt to enable ESLint rule in #20905 didn’t work as expected 🙁

It looks like the bug was fixed and it has landed in Gutenberg. Next step is to move it to the ESLing plugin.

@gziolo gziolo removed the Needs Dev Ready for, and needs developer efforts label May 13, 2020
@gziolo gziolo self-assigned this May 13, 2020
@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label May 13, 2020
@ocean90
Copy link
Member

ocean90 commented Feb 5, 2021

@gziolo Was it expected that we now have to add every @WordPress package used in block scripts as a dev dependency? Asking because the rule now produces these errors:

/wordpress-plugin-boilerplate/assets/js/src/blocks.js
  4:35  error  Unable to resolve path to module '@wordpress/blocks'  import/no-unresolved

/wordpress-plugin-boilerplate/assets/js/src/blocks/example/index.js
  4:20  error  Unable to resolve path to module '@wordpress/i18n'  import/no-unresolved

@gziolo
Copy link
Member

gziolo commented Feb 5, 2021

Yes. It might be too strict though. We can exclude those dependencies that are listed as webpack externals and resolved as globals in WordPress. I had a similar discussion on WordPress Slack. What do you think about it? I didn't anticipate it being problematic.

By the way, @wordpress/create-block installs all dependencies used in the code. It should follow whatever we decide next.

@ocean90
Copy link
Member

ocean90 commented Feb 5, 2021

Not sure, I think they should be excluded but I also see the benefit of having them explicitly defined as a dependency. One issue I see is the versioning because they version installed might not match the version included in the minimum supported WordPress version. With automated package updates (Dependabot) it's too easy to install a new version.
Might also be not be so nice to install all the dependencies which aren't actually used just to make a rule happy while it's technically a false-positive.

Could you link to the Slack discussion please?

@nerrad
Copy link
Contributor

nerrad commented Feb 5, 2021

Here's a link to the discussion that happened in Slack: https://wordpress.slack.com/archives/C5UNMSU4R/p1612354700163900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Tool] ESLint plugin /packages/eslint-plugin [Tool] WP Scripts /packages/scripts [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants