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

Chore: Upgrade @wordpress/scripts to the latest version #87

Closed
wants to merge 2 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 29, 2019

npm run lint-js still doesn't work.

I'm seeing:

/Users/gziolo/PhpstormProjects/gutenberg-examples/test/examples.js
  12:1  error  'test' is not defined    no-undef
  17:2  error  'expect' is not defined  no-undef
  21:1  error  'test' is not defined    no-undef
  26:2  error  'expect' is not defined  no-undef

✖ 4 problems (4 errors, 0 warnings)

@gziolo
Copy link
Member Author

gziolo commented Aug 29, 2019

@ntwb - I would appreciate your help here. This fixes those errors:

diff --git a/.eslintrc.js b/.eslintrc.js
new file mode 100644
index 0000000..4fa3476
--- /dev/null
+++ b/.eslintrc.js
@@ -0,0 +1,4 @@
+module.exports = {
+       root: true,
+       extends: [ 'plugin:@wordpress/eslint-plugin/recommended' ],
+};

So there needs to be some issue with how config files are processed when loaded from node_modules folder.

Related PR in ESLint: eslint/eslint#11554.

Update 1: I know what happens, this line causes issues:

https://github.com/eslint/eslint/blob/00d2c5be9a89efd90135c4368a9589f33df3f7ba/lib/cli-engine/config-array-factory.js#L485

as this translates into:

criteria:
     { includes: [Array],
       excludes: null,
       basePath:
        '/Users/gziolo/PhpstormProjects/gutenberg-examples/node_modules/@wordpress/scripts/config' },

when referencing the config inside node_modules folder.

Update 2: A hack inside ESLint codebase in the line shared above solves the issue:

const basePath = filePath && ! filePath.includes( 'node_modules' ) ? path.dirname(filePath) : cwd;

I think we will have to open PR against ESLint to see whether it can be fixed upstream.

Update 3: A similar issues opened in ESLint eslint/eslint#12032

@gziolo
Copy link
Member Author

gziolo commented Sep 17, 2019

I filed a new issue in ESLint: eslint/eslint#12278.

@swissspidy
Copy link
Member

Wanna jump directly to v5 of the package?

@gziolo
Copy link
Member Author

gziolo commented Sep 17, 2019

Wanna jump directly to v5 of the package?

Yes, v5 would be the best way to go. This one was blocked by ESLint bug which prevents us from using ESLint config from npm package directly for overrides applied to work with all files which contain tests. We can skip that part though and update ESNext examples to start using new asset file together with import statements to show how it simplifies everything :)

@gziolo
Copy link
Member Author

gziolo commented Sep 17, 2019

With b74542f this PR uses the latest version of @wordpress/scripts. I plan also to integrate wp-scripts env and connect asset files with PHP code to let webpack handle the most error-prone part.

@gziolo
Copy link
Member Author

gziolo commented Sep 20, 2019

Replaced with #89.

@gziolo gziolo closed this Sep 20, 2019
@gziolo gziolo deleted the update/upgrade-wordpress-scripts branch September 20, 2019 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants