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

Glob gets different results depending on where it’s launched #12

Closed
pepelsbey opened this issue Sep 16, 2023 · 4 comments
Closed

Glob gets different results depending on where it’s launched #12

pepelsbey opened this issue Sep 16, 2023 · 4 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@pepelsbey
Copy link

pepelsbey commented Sep 16, 2023

Thank you for helping to figure out the multiple ignore messages in #11, it worked!

But then I noticed that the tool behaves differently with identical parameters. The only difference is where I launch it: via npm script or via npx in CLI.

Npm script

I have a script that goes like this:

"html": "w3c-html-validator dist/**/*.html '--ignore=/^Element “style” not allowed as child of element “div” in this context|^Bad value  for attribute “src” on element “iframe”/'",

And the tool is in the dependencies, of course:

"w3c-html-validator": "^1.4.0"

But it sees only 4 files with the dist/**/*.html mask:

$ npm run html

> html
> w3c-html-validator dist/**/*.html '--ignore=/^Element “style” not allowed as child of element “div” in this context|^Bad value  for attribute “src” on element “iframe”/'

[17:50:01] w3c-html-validator files: 4
[17:50:01] w3c-html-validator ✔ pass dist/404/index.html 
[17:50:02] w3c-html-validator ✔ pass dist/about/index.html 
[17:50:02] w3c-html-validator ✔ pass dist/articles/index.html 
[17:50:03] w3c-html-validator ✔ pass dist/projects/index.html

Npx launch

But when I use exactly the same set of parameters via npx, it sees all 42 files (which is correct):

$ npx w3c-html-validator dist/**/*.html '--ignore=/^Element “style” not allowed as child of element “div” in this context|^Bad value  for attribute “src” on element “iframe”/'

[17:50:23] w3c-html-validator files: 42
[17:50:24] w3c-html-validator ✔ pass dist/about/index.html 
[17:50:25] w3c-html-validator ✔ pass dist/404/index.html 
[17:50:25] w3c-html-validator ✔ pass dist/articles/2/index.html 
[17:50:25] w3c-html-validator ✔ pass dist/articles/a-third-breath/index.html 
[17:50:26] w3c-html-validator ✔ pass dist/articles/conditionally-adaptive/demo/index.html 
[17:50:26] w3c-html-validator ✔ pass dist/articles/conditionally-adaptive/index.html 
[17:50:27] w3c-html-validator ✔ pass dist/articles/eleventy-css-js/index.html
…etc.

I’d love to use it via npm scripts, but I’m not sure why the same dist/**/*.html glob gives me different results. I tried some variations, but no luck so far.


It’s macOS 13.5.2, Node v18.17.1, npm 9.6.7 (but I got the same results with Node v16)

@pepelsbey pepelsbey changed the title Glob in the CLI acting differently depending on where it’s launched Glob gets different results depending on where it’s launched Sep 16, 2023
@dpilafian dpilafian added the bug Something isn't working label Sep 17, 2023
@dpilafian
Copy link
Member

The glob is interpreted by the shell before it is processed by w3c-html-validator. The expanded command may be so long that in some situations files are getting dropped.

However, w3c-html-validator will recursively search a folder for HTML files automatically, so maybe (hopefully) this issue is moot.

What happens if you replace dist/**/*.html with dist in both commands?

@pepelsbey
Copy link
Author

What happens if you replace dist/**/*.html with dist in both commands?

It works with all 42 files in both cases when just the dist is passed. I’m going to use that, but I think it’s something worth investigating or at least mentioning in the docs. I’ve never encountered a glob that gives different results before.

@nibynool
Copy link

nibynool commented Oct 2, 2023

I'm encountering a similar issue with globbing and can probably shed some light on this.

npm will use sh to execute commands whilst the MacOS command line is usually running zsh (or bash on older OS versions). sh doesn't support globbing and interprets ** as *, thus you get a difference between the two execution methods.

The recommendation from a number of other packages is to quote the glob pattern. This would work, except that line 39 and 40 of cli.js append **/*.html (with an optional leading slash). In @pepelsbey's issue adding quotes means the glob would become dist/**/*.html/**/*.html.

In my situation a large number of my files do not have an .html extension. If I use the npx method of running the command zsh will interpret public/**/* and supply a list of filenames (so each file in the list is validated). If I run it throughnpm without quotes only the files matching public/*/* get validated. If I run it through npm with quotes it throws an error as public/**/* doesn't exist as a directory so it can't work out how to handle it.

Ideally support for both folder and/or glob patterns would be good. What if a PR were created to add a command line option to override the default glob pattern? Would this likely be accepted as a concept?

@dpilafian dpilafian added the enhancement New feature or request label Oct 2, 2023
@dpilafian
Copy link
Member

dpilafian commented Oct 2, 2023

While w3c-html-validator uses globs internally, it currently does not explicitly support globs passed in when quoted. Glob is only used on folders so there's no risk of appending '**/*.html' to a passed in glob parameter. Instead, the glob pattern will be interpreted as a file and fail because it's not found.

Adding explicit glob support (for quoted glob parameters) makes sense. Passing all the files through globSync() first seems like it should do the right thing.

Maybe something like this:

const globOptions = { ignore: '**/node_modules/**/*' };
const getFilenames = () => 
   files.map(file => globSync(file, globOptions)).flat().map(expandFolder).flat().filter(keep).sort();
   //                ^^^^^^^^^^^^^^^^^^^^^^^^^^^ first round of processing

I suspect a new CLI option is not needed. Although, it is interesting that the popular rimraf project added a --glob CLI option recently. I don't know why. The project supported globs for many years without requiring --glob. Maybe it's a way of subtlety saying, don't use magic unless you're a wizard.

A PR would be great.

dpilafian added a commit that referenced this issue Nov 5, 2023
dpilafian added a commit that referenced this issue Nov 10, 2023
dpilafian added a commit that referenced this issue Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants