Add Webpack and build a browser-first version

[stackmystack]

About

Adds webpack and builds browser-ready js file under build/.

The webpack config was taken verbatim from the eslint project.

A new entry-point is used to generate the file: web.js, and its sole purpose is to prepend the no-unsanitized/ namespace in front of the method and property rules, so we could resolve them nicely from the browser.

Background

I must confess, I am no JS developer, but I need the rules in the browser. eslint already supports webpack and builds a web version.

The produced build/no_unsatize.js can be loaded alongside eslint.js:

    <script src="./eslint.js"></script>
    <script src="./no_unsanitize.js"></script>

And now you can do:

let rules = {
  env: {
    browser: true,
  },
  rules: {
    semi: 2,
    "no-unsanitized/method": "error",
    "no-unsanitized/property": "error",
  },
};

const linter = new eslint.Linter();
linter.defineRules(no_unsanitize.rules);
console.log(linter.verify("foo.innerHTML = input.value", rules));

The result would be:

[
    {
        "ruleId": "no-unsanitized/property",
        "severity": 2,
        "message": "Unsafe assignment to innerHTML",
        "line": 1,
        "column": 1,
        "nodeType": "AssignmentExpression",
        "endLine": 1,
        "endColumn": 28
    },
    {
        "ruleId": "semi",
        "severity": 2,
        "message": "Missing semicolon.",
        "line": 1,
        "column": 28,
        "nodeType": "ExpressionStatement",
        "messageId": "missingSemi",
        "fix": {
            "range": [
                27,
                27
            ],
            "text": ";"
        }
    }
]

EDIT: forgot the missing linter.defineRules call.

[mozfreddyb]

I had no idea that eslint works in the browser. I'd be more interested in moving things forward if I had a better understanding of what you're trying to achieve and how you got there.

Besides, we also need to make sure that existing CI is happy (I just allowed the tests to run and they failed).

[stackmystack]

I had no idea that eslint works in the browser.

They don't advertise it :) And their doc, like almost all JS projects nowadays, assume you're writing JS with node.

I'd be more interested in moving things forward if I had a better understanding of what you're trying to achieve and how you got there.

Fair enough.

What?

I want to run eslint in the browser, to lint javascript sources, and I don't know which rules are available at runtime.

In detail:

1. I'm not developing a node package.
2. I'm targeting the browser.
3. I have to work with a "vanilla" eslint for the web and use/apply rules depending on their availability at runtime.

So the browser would pull eslint, then pull, depending on various conditions including user input, a rule package, one of them is no-unsanitized.

I want to be able then to lint some JS text using eslint and the runtime-defined rules.

The first thing I tried to do was to transpile this package to the web:

1. I defined webpack.config.js by copying it from eslint
2. I pointed it to index.js.

I defined my HTML as described in the PR, then I tried to do this:

let rules = {
  env: {
    browser: true,
  },
  rules: {
    semi: 2,
    "no-unsanitized/method": "error",
    "no-unsanitized/property": "error",
  },
};

const linter = new eslint.Linter();
console.log(linter.verify("foo.innerHTML = input.value", rules));

It failed miserably.

What's happening?

We have to understand what's happening when we transpile eslint and this plugin:

1. eslint gets transpiled into a huge IIFE, and all its exported symbols are accessed by the global var eslint.
2. no-unsatnitized gets transpiled similarly, and it's accessed through the no_unsantize global var.

How can eslint recognize no_unsanitize as a plugin? We have to use its public API.

Step 1: define no_unsanitize

As in:

linter.defineRules(no_unsanitize.rules)

PS: I edited this PR's example to reflect that.

Now all the rules in no_unsanitize.rules are recognisable by eslint.

I ran my example, and it also failed miserably. Same problem actually: eslint couldn't see the rules.

With a simple inspection of the Map of linter.getRules(), you'd clearly notice that the linter.defineRules call was successful, the Map has 2 more entries, and they're called method and property. Totally unqualified.

So if we use the same example from this PR, BUT we defined rules like this:

let rules = {
  env: {
    browser: true,
  },
  rules: {
    semi: 2,
    "method": "error",
    "property": "error",
  },
};

… then our call to linter.verify will actually trigger the rules and return the correct results. No complaint at all.

But that's problematic, obviously:

1. The divergence from the code you'd use when developing with node vs the web is bad for the users for all the confusion it might create.
2. The possibility of clashes with rules from other projects is a sad situation to be in. I have yet to hear of a single developer who enjoys handling name clashes for plugins they didn't write of a framework they're using.

Step 2: define web.js

This is where I re-export parts of the exported object from index.js, with the proper qualification. The example of this PR now works.

Final remarks

Again, I'd like to stress out that I am not an experienced JS developer. And I understand that this is a niche use case. So if there's a cleaner way to achieve same results (maybe some webpack shenanigans that allow us to manipulate the exported object before packing it?), I'm all ears.

I hope this explanation was clear. Let me know if you need more info.

Besides, we also need to make sure that existing CI is happy (I just allowed the tests to run and they failed).

Thanks @mozfreddyb. I will check it out out ASAP.

[mozfreddyb]

A lot of the changes and impact and warnings that I see from your pull request stem from, let's say, noisy changes: Indentation, package updates, dependencies being added etc. It's a bit hard for me to review and assess the actual impact of your change with this noise. I suggest you try fix this warnings and this will make it easier for me to discuss possible next steps. That being said, I want to acknowledge that making the change easier to review is a bit of work and we might still not want to take your change after all.

You said you're not an experienced JS developer and that's fine. We can happily keep that PR open for a longer while, if it takes a bit of work. Please also note that you'll be able to iterate more quickly if you run the tests locally. They are (relatively) fast: npm run lint to check for syntax issues and then npm run test to make sure our test suite passes.

[stackmystack]

Yes my editor did not understanding the JS environment. They're all indentation issues.

Everything fixed, tests are passing, and I forced pushed this branch for you to review.

Cheers.

[mozfreddyb]

Sorry for the delays. I am basically maintaining this project in my spare time. My actual job duties are currently shifting away from this project.

Review-wise this look good. Can you deploy this in a personal repository (branch) of yours for easier testing from my end? Thanks!

[stackmystack]

No problem at all. But I didn't get your comment:

Can you deploy this in a personal repository (branch) of yours for easier testing from my end?

It's already on my personal branch in my repo.

[mozfreddyb]

Hey, sorry about the long delay. I think this is mostly good to go. Before I merge, I also asked for review from another collaborator.

@stackmystack Can you also add webpack build as a separate CI step in our github actions workflow config? This will help us make sure the build does not break in upcoming changes

[stackmystack]

I renamed the webpack step to build because you may want to use another tool in the future, so now it's tooling agnostic.

I also rebased, then added the requested step to the CI, and ran it in my repo. It passes.

[mozfreddyb]

@rpl Please review

[mozfreddyb]

if we should consider adding the bundled file in our npm package or not (eslint doesn't seem to be included its webpack bundle neither to be honest and so even if we include our bundled file it may still not be that helpful for making the entire setup to be easier in practice)

No, not yet. My main use case was really just to have a demo that people can use to quickly test the rule and file bugs. I think this could live somewhere e.g., at https://mozilla.github.ui/eslint-plugin-no-unsanitized/demo - I'd rather not make this part of what we ship.

things that we release without test coverage have the annoying tendency of breaking without letting us know :-), and so it would have been nice if we could have covered this with even just a single small smoke test loading and exercising the bundled plugin to confirm it does work as expected, but the fact that eslint doesn't include their bundle in their npm package would make that test a bit more annoying to write.

e.g. we would need to do a shallow clone of eslint repo, install its dependency and build and bundle, then assemble that with our own bundled file and run the smoke test, e.g. inside a jsdom-based mocked browser environment)

What is described above is not impossible, it is pretty doable, but require much more work than it would have required if we could have copied eslint built bundle from the eslint package instead of having to build it as part of such a test case.

And so I wouldn't block on that test case. @mozfreddyb wdyt? does that sound reasonable to you too? (don't block on the additional test case with the rationale described above)

One of the reasons I'd want us to start off with a "demo" use case rather than making this a supported thing we ask people to use.

Instead I'd say we could still add a small section in the README.md file to explain how to use the bundled file, given that requires some knowledge to work correctly (and differs enough from how you would setup the plugin when using eslint from nodejs and execute it from the command line). @mozfreddyb wdyt? you would be willing to include a brief section about that in the repo README.md file?

I'm not sure that's even necessary in case we just use it "internally" as suggested above.

[rpl]

if we should consider adding the bundled file in our npm package or not (eslint doesn't seem to be included its webpack bundle neither to be honest and so even if we include our bundled file it may still not be that helpful for making the entire setup to be easier in practice)

No, not yet. My main use case was really just to have a demo that people can use to quickly test the rule and file bugs. I think this could live somewhere e.g., at https://mozilla.github.ui/eslint-plugin-no-unsanitized/demo - I'd rather not make this part of what we ship.

things that we release without test coverage have the annoying tendency of breaking without letting us know :-), and so it would have been nice if we could have covered this with even just a single small smoke test loading and exercising the bundled plugin to confirm it does work as expected, but the fact that eslint doesn't include their bundle in their npm package would make that test a bit more annoying to write.

e.g. we would need to do a shallow clone of eslint repo, install its dependency and build and bundle, then assemble that with our own bundled file and run the smoke test, e.g. inside a jsdom-based mocked browser environment) What is described above is not impossible, it is pretty doable, but require much more work than it would have required if we could have copied eslint built bundle from the eslint package instead of having to build it as part of such a test case. And so I wouldn't block on that test case. @mozfreddyb wdyt? does that sound reasonable to you too? (don't block on the additional test case with the rationale described above)

One of the reasons I'd want us to start off with a "demo" use case rather than making this a supported thing we ask people to use.

Instead I'd say we could still add a small section in the README.md file to explain how to use the bundled file, given that requires some knowledge to work correctly (and differs enough from how you would setup the plugin when using eslint from nodejs and execute it from the command line). @mozfreddyb wdyt? you would be willing to include a brief section about that in the repo README.md file?

I'm not sure that's even necessary in case we just use it "internally" as suggested above.

@mozfreddyb sounds good to me, as I mentioned in my comments I was already considering all of those as non-blocking issues from my perspective, let's just aim to cover the few small tweaks proposed and then merge this.

[mozfreddyb]

@stackmystack Sorry it took us so long. Are you still interested in maintaining this Pull Request and bringing it to completion?

[stackmystack]

@stackmystack Sorry it took us so long. Are you still interested in maintaining this Pull Request and bringing it to completion?

Hello @mozfreddyb, I am on vacation, and will not be available before 5 weeks. So If you can wait until then, I'd happily spare some time to maintain it. But I guess you guys agreed on little-to-no-modifications AFAICT, so If you feel you want to merge early, please be my guest, the branch is yours :)

[mozfreddyb]

@stackmystack Please take a look at some of the suggested changes. We can easily wait ~5 more weeks :)

[stackmystack]

@mozfreddyb @rpl

I integrated as much of your comments and suggestions as possible.

Ready for a second iteration of review, since I added comments/skipped some suggestions.

I will also skip the smoke test part since it's a lot of effort given that my day job requires more attention on different topics, so it wouldn't be nice of me to promise I'd do it when I can't be sure about it.