fluid-project / fluid-lint-all

Consolidated linting logic free from any particular build technology
BSD 3-Clause "New" or "Revised" License
0 stars 5 forks source link

Improper use of `fluid.flatten` results in sources not being included in checks. #12

Closed greatislander closed 3 years ago

greatislander commented 3 years ago

Describe the bug

In my work on integrating fluid-lint-all into https://github.com/fluid-project/infusion/pull/1014, I've encountered a bug related to the stylelint check where SCSS files are not included properly.

To reproduce

Steps to reproduce the behavior:

  1. Check out https://github.com/greatislander/infusion/tree/FLUID-6555.
  2. Install dependencies.
  3. In node_modules/fluid-lint-all/src/js/stylelint.js, insert console.log(filesToScan); after line 35.
  4. Run npm run lint.
  5. Observe that the logged list of files online includes .css files.

Expected behavior

The files list includes .scss files in src/framework/preferences/css/sass.

Technical details

Desktop

Additional context or notes

I tried modifying node_modules/fluid-lint-all/src/js/lint-all.js and noticed that if I changed line 189 from:

            "includes": "@expand:fluid.flatten({that}.options.config.sources.css, {that}.options.config.sources.scss)",

To:

            "includes": "@expand:fluid.flatten({that}.options.config.sources.scss, {that}.options.config.sources.css)",

I encountered the opposite problem— ONLY .scss files were linted and all .css files were skipped. These leads me to think that there's something wrong with the way fluid.flatten is merging the two source lists.

jobara commented 3 years ago

It appears that fluid.flatten takes a single argument. In how it appears to work in the provided example, you could just do an array concatenation or pass in an array of sources to fluid.flatten.

amb26 commented 3 years ago

That's right - the call to fluid.flatten is incorrect since this function accepts an array rather than an argument list - https://docs.fluidproject.org/infusion/development/coreapi#fluidflattenarray This could be written out as a long-form expander as

"includes": {
    expander: {
        func: "fluid.flatten",
        args: ["{that}.options.config.sources.scss", "{that}.options.config.sources.css"]
    }
}

or else as a dedicated function - it's not like fluid.flatten does very much

greatislander commented 3 years ago

@amb26 thanks for clarifying— looks like this would cause problems with the JSON check's configuration as well:

https://github.com/fluid-project/fluid-lint-all/blob/aa7e2ef6f21224e5a586db23c84259c0f02f3307/src/js/lint-all.js#L91

the-t-in-rtf commented 3 years ago

I didn't end up making the first argument an array, so we have the same problem in a different form.