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

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

Closed
Standard8 opened this issue Jun 21, 2017 · 13 comments
Closed

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

Standard8 opened this issue Jun 21, 2017 · 13 comments

Comments

@Standard8
Copy link

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
Copy link
Owner

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

@franko-franicevich
Copy link

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
Copy link
Author

@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:

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
Copy link
Owner

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:

  • Function declaration are not hoisted accross script tags, so this throws a ReferenceError:
  <script>
    foo();  // reference error
  </script>
  <script>
    function foo() {}
  </script>
  • Modules are also using the window as global scope, so this works:
  <script>
    class Foo {}
  </script>
  <script type="module">
    new Foo();
  </script>

My approach would be to parse each script tag to determine which global variables are used and which variables are exported to the global scope, then actually running ESLint rules by specifying which variables are OK to be unused (because they are used in a script tag coming after) or undeclared (because they are declared in a previous script tag).

I commited a WIP on 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
Copy link
Author

@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
Copy link
Owner

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
Copy link
Author

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

@BenoitZugmeyer
Copy link
Owner

@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
Copy link
Author

@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
Copy link
Owner

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

@Standard8
Copy link
Author

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
Copy link
Author

@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
Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants