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

Add xo linter, ava test framework, webpack build tool, README #147

Merged
merged 8 commits into from
Jul 12, 2018

Conversation

dscrobonia
Copy link
Contributor

This adds xo as our linting tool, ava as our testing framework, webpack to transpile our Vue, and creates a simple README.

I ran the linter and I loved the results from it. This change doesn't include that run or the automatic changes. I'm not sure the best way, but it may be just to run it, make the 10,0000 changes and do it all at once. Either way that's not a part of this PR.

I also only included a sample test file, but ava tests are very simple to write.

Finally, although this PR adds webpack, I still need transform our html, js, and css files into Vue components that would then be transpiled by webpack back into usable js.

Copy link
Contributor

@Pamplemousse Pamplemousse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent first step!

README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@dscrobonia
Copy link
Contributor Author

Just added some precommit hooks to automatically lint, fix any lint issues, and then run tests. : ) Unleash the robots!

@psiinon
Copy link
Member

psiinon commented Jul 5, 2018

OK, this sounds really good :)
Just to make sure I'm clear on this - am I right in thinking:

  • If someone uses webpack then we dont need to disable the HUD CSP
  • We can still just edit the JS files and test them without webpack as long as keep using the 'dev' option

README.md Outdated Show resolved Hide resolved
Copy link
Member

@psiinon psiinon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think the instructions should be in a wiki page. Other than that would just like answers to the questions I've just posted ;)

@dscrobonia
Copy link
Contributor Author

If someone uses webpack then we dont need to disable the HUD CSP

Webpack will allow us to keep the HUD CSP. The way webpack will work is that we're going to rewrite all of our html,js,css files into .vue files that define all three of those aspects for a single "component". Then we'll use webpack to transpile those .vue files into pure js which is what the HUD will actually use. And because we've transpiled into nonsense javascript before we load them in the browser we don't need to interpret Vue in the browser, which won't trigger the CSP. (Better explanation and links here)

We can still just edit the JS files and test them without webpack as long as keep using the 'dev' option

There won't be any js source files for the UI, just vue components. (I believe) Which means we can build everytime we make a change to see the results. But I'm pretty sure there are also development tools (which I believe is what you're referring to as the 'dev' option) that simplify this process so you don't have to build everytime.

@psiinon
Copy link
Member

psiinon commented Jul 6, 2018

Is there a way we can keep the current way we work as a 'dev' option?
I'm fine with requiring webpack for 'full on' HUD development.
But I'd really like us to still support the way we work now as a non default option.
So this would mean switching on a HUD 'dev' mode which:

  • Means the raw JS files are used instead of Vuejs components
  • Turns off the CSP element which prevents exec from being used
  • Possibly stops the HUD from caching the JS files so that we can change them 'live'

I really want to make 'hacking the HUD accessible' and being able to make changes to it without having to install any other software (like webpack) is my goal. For 'serious' HUD development I'm absolutely fine with us requiring extra tools to be installed.

Does that make sense?

@dscrobonia
Copy link
Contributor Author

What are the final steps I need to take to get this through? I was thinking I'd remove the husky dependency, and then it should be good to go ya? Or @psiinon were there other changes i need to make on this PR

@psiinon
Copy link
Member

psiinon commented Jul 7, 2018

For a start I need to make sure I can actually build the HUD with these changes applies, and work out if they have any other unexpected consequences.
Are these the right instructions? https://github.com/zaproxy/zaproxy/wiki/FAQapikey
I'd like the ant task to use npm so that those of use who use eclipse have an option not to drop out to the command line, even if its just using an 'exec' task.

@dscrobonia
Copy link
Contributor Author

Ah definitely. Yeah I can glue ant and npm together so that you can invoke the npm commands from ant. To be clear, in this PR we are not using the webpack build pipeline yet. I was going to implement that along with refactoring to components. For now it just includes the library and implements the linting & testing that we want. So I'll expose the linter & testing to ant. And then expose webpack later. :)

I'll also add further instructions on the wiki. Did you mean to link that FAQ for the API key?

@dscrobonia
Copy link
Contributor Author

@psiinon let me know what else I need to add here or if I missed something. : ) I know @Pamplemousse has some changes depending on this one.

build/build.xml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@thc202
Copy link
Member

thc202 commented Jul 12, 2018

Which version of npm should we use?

I was expecting an additional Ant target to execute npm run build (which could be used in another target that calls that one and builds/deploys the add-on) or that's for later?

@Pamplemousse
Copy link
Contributor

If I read correctly, there is a way to specify a node and npm versions for our package: docs.npmjs.com/files/package.json#engines.

I think the Ant target does not have to be on this PR ; But it has to follow soon.
It's a must for one to run ant -f build/build.xml deploy-hud and have everything working without any more steps.

@psiinon
Copy link
Member

psiinon commented Jul 12, 2018

Sorry, no, I meant to link to https://github.com/psiinon/zap-hud/wiki/Developing-with-the-HUD
So I tried the ant tasks, and they failed, first because I didnt have npm installed and then for xo and then ava.
The wiki page should cover the dependencies and link to suitable pages so that people can easily find what to install and install them in one go.
Even though I've installed ava I've getting errors like:

Error: Cannot find module 'ava'

lint-all-hud works, but not surprisingly reports loads of errors. Thats fine, we should def address these.

lint-staged-hud fails with:

[exec] sh: lint-staged: command not found

@psiinon
Copy link
Member

psiinon commented Jul 12, 2018

OK, my bad, I hadnt run "npm install" as details in the instructions :D

Copy link
Member

@psiinon psiinon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this, although you should fix the comment as per thc202's suggestion.
Would also be good to document how we can fix the lint errors as efficiently as possible, but that can be in the wiki so doesnt have to be part of this PR.

@dscrobonia
Copy link
Contributor Author

Thank you everybody for your reviews!

@thc202 I'll fix those two notes you added.

As for adding npm run build to the ant task, there currently isn't anything for npm to "build" yet. Any building required to make the HUD work is already in the ant task. The npm run build command will be used for when we convert all of the UI's to .Vue files that need to be transpiled into JS. Until that point, we don't need to run it yet. Let me know if I'm missing something.

@psiinon if we run ant lint-staged it will actually automagically fix any of the linting errors it is able to! (most of them can be fixed) but that would be a really good note to add to the wiki. you can also run xo --fix.

@dscrobonia
Copy link
Contributor Author

Oh and for npm versions, I'm not sure what version we should require. We can follow @Pamplemousse suggestion to add it to the package.json file.

@thc202
Copy link
Member

thc202 commented Jul 12, 2018

Thank you!

@psiinon psiinon merged commit 6925acb into zaproxy:master Jul 12, 2018
@thc202 thc202 deleted the add-webpack branch December 1, 2018 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants