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

Flow comments are not stripped from files in `lib` #2322

Closed
IanVS opened this Issue Feb 6, 2017 · 15 comments

Comments

7 participants
@IanVS

IanVS commented Feb 6, 2017

Describe the issue. Is it a bug or a feature request (new rule, new option, etc.)?

Bug.

It seems that flowtype comments are not being stripped correctly, and are ending up in files in lib (which was causing my own app's flow checks to fail). It was good, because now I realized I was checking node_modules which I shouldn't be doing, but I think you should also be stripping the types from the files that are distributed through npm, right?

Which rule, if any, is this issue related to?

core

Which version of stylelint are you using?

7.8.0

What did you expect to happen?

There should be no flow errors from node_modules/stylelint/lib

What actually happened (e.g. what warnings or errors you are getting)?

I had errors such as:

node_modules/stylelint/lib/utils/isStandardSyntaxAtRule.js:10
 10: module.exports = function (atRule/*: postcss$atRule*/)/*: boolean*/ {
                                          ^^^^^^^^^^^^^^ identifier `postcss$atRule`. Could not resolve name
@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 6, 2017

@IanVS: We are using Flow's comment syntax precisely so we do not have to strip out the annotations before publication, but can instead publish the code as-is.

As far as I know, there is nothing wrong with the Flow-comments themselves being published. Instead, the problem seems to be that our internal declarations, defined in https://github.com/stylelint/stylelint/tree/master/decls, are not being picked up by Flow when your module that uses stylelint runs a check.

I'm not sure what the best way to handle this is, but I know that I do not want to compile stylelint before publication —— any recommendations appreciated.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 6, 2017

I'm not sure what the best way to handle this is ... any recommendations appreciated.

Seconded. Anyone know the recommended approach to this?

Do we need to handle it, or do we just rely on consumers not running flow on their dependencies within node_modules?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Feb 6, 2017

We don't include tests in our package. Why should we include other development related only files? No one expects tests to be present in an installed npm package. Same thing goes with Flow.

For example, normalize.css could not pass user's stylelint's config. But if it's installed as an npm package, this shouldn't be a user's concern.

Another example, I don't think anyone run ESLint over node_modules.

@IanVS

This comment has been minimized.

IanVS commented Feb 6, 2017

I'm a beginner with Flow, but it seems like it's actually a good thing to run it on dependencies, so that Flow can check that I am calling APIs with the correct types of arguments, for example.

I think what you want to do is declare your types. This way, other projects can also use the types you've set up. There is some information at https://flowtype.org/docs/declarations.html.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 6, 2017

@IanVS Since you already have a project set up, would you mind giving declare a quick shot and seeing if it works?

My suspicion is that it won't, though, because our declaration files are only included via .flowconfig, so when stylelint is invoked as a node_module Flow probably will not find that file. Maybe we need to define all of those declarations with Flow-comment syntax within a file that gets required. That might be a little awkward, but not too bad.

@IanVS

This comment has been minimized.

IanVS commented Feb 6, 2017

@davidtheclark it seems your suspicions are confirmed. Adding the .flowconfig and decls into my node_modules does not prevent the errors.

This stackoverflow answer suggests two courses of action:

  1. Users can modify their own .flowconfig files to point to the library's declarations
  2. Rely on flow-typed.

To point 2, have you considered contributing a definition to https://github.com/flowtype/flow-typed? I think that might actually be the most straightforward solution. Although it does seem strange to me that there's no good way for packages to expose their own definitions. 😕

@chrislloyd

This comment has been minimized.

chrislloyd commented Feb 6, 2017

One option is to use gen-flow-files to ship .flow files with the package. Or use flow-copy-files to copy the source to .js.flow files specifically for flow to pick up. I'd recommend against using flow-typed.

In the mean time can we please get a build of stylelint with flow comments stripped?

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 18, 2017

I think there are two pretty good options here:

  • Add a line to [ignore] that ignores stylelint: .*node_modules.*/stylelint.*.
  • Add a line to [libs] that pulls in stylelint's declarations: ./node_modules/stylelint/decls.

If you don't care about stylelint's Flow definitions, try the first. If you are using stylelint's API and do want those definitions, try the second.

@matteocng

This comment has been minimized.

matteocng commented Feb 19, 2017

Copying the ./decls folder manually (it is not published to npm) to ./node_modules/stylelint/decls and pulling the declarations like @davidtheclark proposed, produces 0 errors on Flow v0.39, even tough I also had to add this line to [options] module.ignore_non_literal_requires=true. Unfortunately I don't know if there's a way to set that option for stylelint only and not for the whole project including it.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 19, 2017

Ugh. Ignoring it seems to be the way to go.

@matteocng

This comment has been minimized.

matteocng commented Feb 19, 2017

Forgot to add that if decls was published to npm, allowing the use of [include] to pull stylelint's definitions, having to add [options] module.ignore_non_literal_requires=true would not mean having to allow non literal requires. They could be easily disallowed with eslint by using eslint-plugin-import with the no-dynamic-require rule enabled, the equivalent of ignore_non_literal_requires === false (Flow's default).

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 26, 2017

@davidtheclark Would including the decls in the next published package help alleviate this issue?

@mAAdhaTTah

This comment has been minimized.

mAAdhaTTah commented Feb 26, 2017

This StackOverflow answer suggests resolving a Flowtype error by adding the package's declarations to their .flowconfig. Shipping the decl folder would allow us to resolve that in the consuming application.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 27, 2017

@jeddy3 Yeah, that's a good place to start. Here are the steps I can think of to alleviate the situation — the best we can do (that I know of):

  • Ship the decl folder so people can use it. Unfortunately they'll still run into the annoyance of #2322 (comment), so ...
  • Change the way we use dynamic requires in augmentConfig.js, cli.js, and getPostcssResult.js so that dynamic requires are isolated and not checked by Flow, so we don't have to include that option.

@davidtheclark davidtheclark self-assigned this Feb 27, 2017

@matteocng

This comment has been minimized.

matteocng commented Feb 27, 2017

I noticed that decls contains declarations for jest and lodash coming from flow-typed. Maybe those two should/could be moved to another folder i.e './flow-typed/npm/' (the default folder when you run flow-typed install), included and used internally but not shipped?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment