BenoitZugmeyer / eslint-plugin-html

An ESLint plugin to extract and lint scripts from HTML files.
ISC License
430 stars 51 forks source link

Scripts are treated as modules, when they aren't. #66

Closed Standard8 closed 6 years ago

Standard8 commented 7 years ago

With version 3.0.0, all <script> tags are treated as modules, when they might not be.

This causes no-undef rules to fail when they currently are fine in the browser.

Is it possible to limit the new functionality to modules only or something?

BenoitZugmeyer commented 7 years ago

I will look into this, maybe we can use the same scope instance for multiple script tags.

franko-franicevich commented 7 years ago

We're also having the same problem under the new version of the plugin - A function or variable defined in an earlier script block and used in a later script block fails in the linter, while working fine in the browser.

I understand that this is a tricky problem, since the reverse is not true :) (i.e., a function declared in script block following that which it is defined in would fail in the browser.)

But I suspect that this is a somewhat common use case.

An option to override the new default would be great. For now we're reverting to the older version.

Standard8 commented 7 years ago

@BenoitZugmeyer I had a little time to start looking at this. I've created a few tests and started looking at the actual issue. So far here is my current branch:

https://github.com/BenoitZugmeyer/eslint-plugin-html/compare/master...Standard8:module-separation

I've worked up a separation in the parser so that it can identify modules vs scripts. My idea was then to somehow see if it is possible to combine the TransformableString / codeParts sections such that all the scripts were merged together, but the modules were separate.

I think that sort of technique could work for #61 as well.

The other alternative that I thought of would have been to do the separation, but when we call verify on ESLint, see if we could get the globals out and as a result pass them as options into the other script sections, but that didn't look so viable.

I'm not sure if I'll have lots of time to look at this soon, so feel free to pick it up, or if you have ideas let me know and I'll see if I can make some time.

BenoitZugmeyer commented 7 years ago

This is a good start, thank you! I worked on this on my side, and I figured out a solution to this issue. Going back to concatenate scripts is not the best approach here, because we'll have the same issues as before, and on top of that I found out it's not how it works in the browser. Two observations:

I commited a WIP on https://github.com/BenoitZugmeyer/eslint-plugin-html/compare/master...share-scope so you can get the idea (I didn't plan to make this public so there is still a lot of bugs and stuff to do), and I'll continue to work on it next week.

Standard8 commented 7 years ago

@BenoitZugmeyer do you need help with progressing further here? I think I can possibly make some time available in the next week or two.

BenoitZugmeyer commented 7 years ago

I'm working on it, I'll try to release something before next week. I think I'll drop support of ESLint < 4.6, because a lot of internal changes have been made in this release, and it would be complicated to support previous versions. I'll prepare a major version of my plugin, with shared scopes and some other minor changes.

Standard8 commented 7 years ago

Ok great. Thank you! If you want some pre-release testing, I'm quite happy to help, just ping me.

BenoitZugmeyer commented 6 years ago

@Standard8 I published v4.0.0-alpha.0, you can try it with npm install eslint-plugin-html@next. See the v4 branch if you're interested. You will need ESLint 4.7+ !

Standard8 commented 6 years ago

@BenoitZugmeyer Thanks. This is looking quite good so far. Unfortunately we're now erroring on a lot of our files with:

Invalid location
Error: Invalid location
    at locationToIndex (/Users/mark/dev/gecko/node_modules/eslint-plugin-html/src/TransformableString.js:22:11)
    at TransformableString.originalLocation (/Users/mark/dev/gecko/node_modules/eslint-plugin-html/src/TransformableString.js:121:19)
    at remapMessages (/Users/mark/dev/gecko/node_modules/eslint-plugin-html/src/index.js:236:27)
    at pushMessages (/Users/mark/dev/gecko/node_modules/eslint-plugin-html/src/index.js:98:11)
    at Linter.verifyWithSharedScopes (/Users/mark/dev/gecko/node_modules/eslint-plugin-html/src/index.js:211:5)
    at Linter.verify (/Users/mark/dev/gecko/node_modules/eslint-plugin-html/src/index.js:129:32)
    at Linter.verifyAndFix (/Users/mark/dev/gecko/node_modules/eslint/lib/linter.js:1086:29)
    at processText (/Users/mark/dev/gecko/node_modules/eslint/lib/cli-engine.js:175:32)
    at processFile (/Users/mark/dev/gecko/node_modules/eslint/lib/cli-engine.js:219:18)
    at fileList.map.fileInfo (/Users/mark/dev/gecko/node_modules/eslint/lib/cli-engine.js:537:20)

I dumped the message out at the start of remapMessages, and I got:

{ ruleId: 'spaced-comment',
  severity: 2,
  message: 'Expected space or tab after \'/*\' in comment.',
  line: undefined,
  column: NaN,
  nodeType: 'Block',
  source: '',
  fix: { range: [ 2, 2 ], text: ' ' } }
{ line: undefined, column: 1 }

The '/*foo' didn't appear in our code, so I assume that's something injected by the plug-in. I turned off spaced-comment and it was then able to lint all the files fine.

I think some of these were files with multiple script segments.

Once the files were being linted, the only thing I noticed that is still an issue is issue #65.

BenoitZugmeyer commented 6 years ago

Okay I published a alpha.1 version. It should fix your issue with spaced-comment because it doesn't use comments anymore.

Standard8 commented 6 years ago

Thank you @BenoitZugmeyer, that looks good (side note: you didn't update @next, but I got it anyway).

The only errors I have now are valid ones (yay!), plus the eol-last issue. The eol-last I'm currently working around with ESLint 4's glob config mechanisms.

Standard8 commented 6 years ago

@BenoitZugmeyer Can I ask if there's any outstanding issues you know of with the alpha.1 version? We're thinking about using that to get us onto ESLint 4, rather than wait for the release.

BenoitZugmeyer commented 6 years ago

Sorry for the wait. I just released the v4, as I can't think of any issue. Let me know how it works for you!