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

[[FIX]] Allow linting of inline module scripts in html file #3492

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rbruckheimer
Copy link

If esversion >= 6, extract the code from a script type=module for
linting. Prior to this commit, was skipping.

If esversion >= 6, extract the code from a script type=module for
linting.  Prior to this commit, was skipping.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9ed8527 on rbruckheimer:extract_from_type_module into 1013d51 on jshint:master.

@jugglinmike
Copy link
Member

Thanks for the patch! There's one high-level change I'd like to see before diving in to the details.

In general, the distinction between "module code" and "script code" is provided "out of band." You can't always tell if how a JavaScript file is intended to be interpreted just from looking at its source. In Node.js, this is communicated either through the package.json file or with the file name extension. In the browser, it's done with the type attribute.

So, when JSHint extracts code from a script tag whose type is "module," then it's definitely linting module code. Since this is a certainty for this case, the developer using JSHint shouldn't have to explicitly configure it with the module: true linting option; JSHint should just enable it automatically.

Does that make sense? If so, I'd recommend updating the extract function to return an array of objects (instead of an array of strings), where each object has one property which holds the source code and another property which holds the type.

(Also, you can remove the changes to the files in the dist directory; we take care of updating those once for each new release of the project.)

@rbruckheimer
Copy link
Author

Hi jugglinmike,
Sorry for the delay. I've taken a closer look at the code, and it's trickier than we thought. In order to properly parse the inline scripts in the html properly, the classic scripts need to be scanned before the module scripts. Also, while module scripts can see variables declared in the classic scripts, when a variable is declared in a module script, only that script can see the variable.
Consider an example file with these scripts:

<script type=module id=a>...</script>
<script type=text/javascript id=b>...</script>
<script type=module id=c>...</script>
<script id=d>...</script>

In order to properly detect undeclared and unknown variables at least two passes would need to be made through this file (one for each module script). The first pass would scan scripts b+d+a and the second through b+d+c.
What makes this tricky and complicated are two things:

  1. We only want to output the errors from b and d once. Suppressing errors on the second pass could be done with ignore tags, but that's kind of hacky.
  2. The line numbers get messed up when you move the module script to the end of the code block.

FWIW I took a look at eslint-plugin-html and it treats each script block as its own sandbox. We can emulate this behavior in JSHint in the above example by making 4 passes through the file (one for each script block), but the results would not be correct.

Here is the code I used to test; there are no errors in the browser:

<html>
<body>
<script>
let x = 5;
</script>
<script type="module">
console.log("x+1=" + (x + 1));
console.log("u=" + u);
</script>
<script>
let u = 40;
console.log("x=" + x);
</script>
</body>
</html>

How would you like to proceed?

@jugglinmike
Copy link
Member

Thanks for looking in to this. You're right about the unexpected challenge.

To accomodate this, don't think we want to lint any of the input more than once, and I expect that we should be linting each in document order. Currently, the extraction logic concatenates all the script sources together. This won't work with module code, so we'll have to process each source separately. That means invoking the JSHint function multiple times.

This is easier said than done. As you point out, the sources are dependent on each other, most notably in the form of global variables. Another by-product of the current implementation is that linting rules are also shared--set unused: true in one script, and it will apply to everything that follows.

To accomodate this, we'll need to collect the results of one invocation of JSHint and provide that as input to the next invocation. There's some support for this already (the data function will tell you the global variables and the linting options from the latest invocation), but I don't know if we currently have enough infrastructure to fully pick up where we left off.

Does this make sense? I'm happy to keep working on this with you, but I also know it's more than you bargained for, so no hard feelings if you want to step back.

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

Successfully merging this pull request may close these issues.

None yet

3 participants