oui

oui copied to clipboard

Currently, lint-es only lints src and src-docs. It would be nice to also have the scripts directory linted by this script too.

Hi, can I take this issue? Thank you!

Hi, can I take this issue? Thank you!

Sure! This should be pretty easy. Feel free to ping if you run into any issues!

I

Hi, can I take this issue? Thank you!

Sure! This should be pretty easy. Feel free to ping if you run into any issues!

Is it just adding one line in the .sass_lint.yml? I am not very familiar with linter..

Nope, this should just be updating the lint-es script in package.json:

https://github.com/opensearch-project/oui/blob/ae0171d5fd0ffb9237367d2d8e351be9d4638623/package.json#L25

Currently we are checking directories {src,src-docs}, but we want {src,src-docs,scripts}. This will also most likely involve running yarn lint-es-fix, as we previously weren't linting that folder, so there is most likely some places where the linter will snag

Ah, that makes sense! Thank you for your instructions! Also right now running yarn lint-es --fix returned some errors. It looks like the .js files in scripts/ should be using import statements instead of require. Should I disable this specific es-lint rule or should I change all the requires?

Should I disable this specific es-lint rule or should I change all the requires?

I don't think we want to disable the ESLint rule, as that will disable it for the entire project, which we don't want. Why don't you test and see if the import statement will work? I'm usually a little scared to use import statements in scripts run directly by Node, but we should be at a late enough version that they work as expected.

I don't think we want to disable the ESLint rule, as that will disable it for the entire project, which we don't want. Why don't you test and see if the import statement will work? I'm usually a little scared to use import statements in scripts run directly by Node, but we should be at a late enough version that they work as expected.

I tried changing them into import, but it looks like it will break a lot of things when running yarn start..... I am using node 18.18.0... I tried setting the package.json file "type": "module" to use ESM... but that will break other things too So now I am not sure what to do.

Yeah I was worrying we would see something like that. Not sure about how to go forward in that case... Looping in @AMoo-Miki and @joshuarrrr

Summary from office hours: let's convert the imports back into the require syntax and figure out how to conditionally suppress the @typescript-eslint/no-var-requires for the scripts directory.