Update npm scripts used for building, testing, etc.

[valentijnnieman]

Hi! This is an update for all the npm scripts we're using.

TL;DR
We should automate linting and formatting. Formatting so that we don't have to ask contributors to use 4 spaces instead of 2 for indentation for example, and so that we ensure our code looks nice and clean. We should run linting locally before the build step, so we prevent linting errors happening in CircleCI (which takes more time). I personally want to automate these things, so I don't have to spend time thinking about it.

More detailed:

• it removes the old build-dev, build-dist and copy-lib scripts that no longer work after updating our webpack config and adds the new build:js and build:py scripts.
• it also adds some dev build options, as well as a new build:watch script. This I personally find very handy when making changes to the code, and wanting to test them in an actual Dash app. This watcher just runs build:all when files have changed.
• it removes lingering webpack & babel config files that are no longer used, as well as
  the Archetype dependency.
• it changes the test script to run our python tests instead of linting (and changes the CircleCI command)
• it adds a new lint command
• it adds a new format command that runs Prettier
• It updates the prepublish hook with running formatting first, then linting, then building (formatting and linting might not be needed here / might be overkill, depending on where we would run it)
• It runs formatting and linting when building JS, this is handy for catching linting errors automatically upon building, especially when using the new build:watch.
• it adds the husky dependency, which allows for git hooks. I've set up the pre-push hook, which fires before every git push.
• it runs formatting and linting on CircleCI, before the build step.
• it adds a new scripts folder with a publish.js script, that can be called using npm run publish-all, and it should get the name & version number from package.json, and publish to both NPM and PyPi
• it fixes the Demo app and the npm start command
• it updates the Readme with relevant scripts

Right now the publish script does npm publish, which isn't enough if you've enabled 2FA. Should we make 2FA the default, i.e. update it to run npm publish --otp?

What I would like to discuss most is the pre-push hook. I very much like automating stuff, because I know myself and know that I sometimes forget stuff. The tests take quite a while to finish however, so I would love to know if you think it is unnecessary to run tests before every git push. I know that some other big open source projects like apollo use this flow, and I know that it never hurts to run tests locally before pushing to Github. I also think it is a good idea to format and lint before pushing, so that the code is clean and readable in a PR.

Another point of discussion are the publish scripts publish-all and publish-pypi. I've left those as-is but I know that they've never really worked for me - I always just run prepublish first, then npm publish and then twine upload dist/*** what are your thoughts on this?

@plotly/dash

[chriddyp]

I know that it never hurts to run tests locally before pushing to Github

In test driven development, to fix a bug I often first write a test that demonstrates the bug (the test should necessarily fail) and then I write the fix in another commit to demonstrate that the bug was fixed. If tests have to pass before I push, then it breaks this development pattern.

However, having format in the githook works for me.

it adds a new lint command
it adds a new format command that runs Prettier

Very nice. Is there any case where format runs but lint fails? If not, then can we only have format and get rid of lint?

It updates the prepublish hook with running formatting first, then linting, then building (formatting and linting might not be needed here / might be overkill, depending on where we would run it)

If formatting and linting are in CircleCI, then we shouldn't need to run them before running prepublish. That is, since we publish off of a passing master branch, then the code should be properly formatted.

It removes the old build-dev, build-dist and copy-lib scripts that no longer work after updating our webpack config and adds the new build:js and build:py scripts.

Can we also update the package.json with the versions of webpack (and remove the archetype). i.e. update it to match https://github.com/plotly/dash-component-boilerplate?

It changes the test script to run our python tests (with a command that I always use - does this work for everyone?) instead of linting

For tests, see the commands that are in circle:

dash-core-components/.circleci/config.yml

Lines 49 to 54 in 29d9f1d

	name: Run tests
	command: |
	. venv/bin/activate
	python --version
	python -m unittest test.test_integration
	python -m unittest test.test_dash_import

. We also used to have pylint in there but it might've gotten removed when @T4rk1n added circle 2 support?

[chriddyp]

Another point of discussion are the publish scripts publish-all and publish-pypi. I've left those as-is but I know that they've never really worked for me - I always just run prepublish first, then npm publish and then twine upload dist/*** what are your thoughts on this?

Yeah, I'd say let's remove these. I'm not sure how to get twine to upload the latest package, so I always run these manually:

git checkout master
git pull origin master
npm run prepublish
python setup.py sdist
npm run publish
twine upload dist/dash-core-components-0.25.1.tar.gz
git tag -a 'v0.25.1' -m 'v0.25.1'
git push origin master --follow-tags

Or perhaps we turn it into a script that substitutes the version number, so we run:

npm run publish-all 0.25.1

and it does all of those things, subsituting the version number where appropriate

[valentijnnieman]

@chriddyp Yeah fair about test driven development. Running tests locally before every push does take a long time too, so I'll remove that for now. I'm actually not sure about if lint catches anything that format doesn't - my thinking was that format won't catch/format ESLint specific rules like no-console, but I'll test that out a bit more. Currently, there's no formatting in CircleCI that I can see - no use of Prettier anywhere that is. Maybe I'm missing something? I got the idea from this PR #277 where the code being pushed wasn't formatted.

I like the npm run publish-all 0.25.1 (and good catch with the tags, keep forgetting that), I'll try to implement that too.

[chriddyp]

Currently, there's no formatting in CircleCI that I can see - no use of Prettier anywhere that is. Maybe I'm missing something?

Yeah, I don't think you're missing anything. We should add prettier, eslint, pylint, and flake8 to the circle config. pylint and flake8 used to be in there I believe.

[T4rk1n]

We also used to have pylint in there but it might've gotten removed when @T4rk1n added circle 2 support?

We only ran pylint on the package __init__, there is almost nothing in there that warrant running pylint. Also with the generated components class files it was failing. I wanted to run it on the tests, but it was failing with a really low score so I left it out.

eslint tests runs in the npm tests. I'm not really familiar with prettier but I added it and the configs from #274, I believe it checks the prettier rules while running eslint. I think formatting should be done by pre-commit hook or running the command locally.

Considering pre-commit hooks, in my experience, they don't work well with IDE, so it needs to be togglable.

It removes the old build-dev, build-dist and copy-lib scripts that no longer work after updating our webpack config and adds the new build:js and build:py scripts.

I have also removed the old build command in #293.

Another point of discussion are the publish scripts

I also don't use them, I do npm publish --otp 2222 because of the 2 factor auth. And do python setup.py sdist then twine upload dist/*.

[T4rk1n]

won't catch/format ESLint specific rules like no-console,

The no-console is in the eslintrc, it didnt catch it because you used window.console.log, it would of catch console.log. I think normally it is stripped out by webpack, but I updated webpack and maybe we're missing a config option.

[bpostlethwaite]

I would prefer not to introduce Husky into our libraries. It is great for in-house application development where devs can agree on tooling. But in OSS where there isn't that level of control forcing git to automagically run when things happen can be dangerous in certain environments.

For example some IDEs configure git hooks already (like my emacs magit IDE)

[valentijnnieman]

@T4rk1n I just used no-console as an example, in that ESLint will catch different errors than Prettier.

@bpostlethwaite Ah ok, that's interesting. I can just add a npm run format step then in CircleCI, if that works just as well (i.e. if that formats the pushed code so that it is formatted how we want it when viewing the changed code in Github, and not when finally merging into master)

I've also removed the old webpack and babel configs, as well as the archetype from deps, since we've upgraded webpack and are no longer using them.

[valentijnnieman]

I'm not sure what's causing the Percy diffs - I guess formatting the Dropdown code changed something, but I don't see what could've caused the difference in rendering. This is what's changed: https://github.com/plotly/dash-core-components/pull/299/files#diff-9022a6ae20c263b738f1e98ca94b74e8

[valentijnnieman]

I've made some changes. I still think it's a good idea to automatically run the formatter and the linter locally, mostly because having the tests fail on CircleCI because you forgot a semicolon somewhere is a bit of a hassle, and might as well run formatting too while you're at it, so that if newcomers make changes we don't have to comment on their coding style, i.e. that we want 4 spaces instead of 2 for indentation. I'm now running formatting and linting as steps before building, when running build:js.

I've also updated CircleCI to run formatting and linting before building, which is maybe unnecessary if we're already doing that in build:js, but it only takes a second and it's good to double check. I've also added a build:watch script, which is handy when making changes you want to check out in an actual Dash app.

[valentijnnieman]

@bpostlethwaite Could you review as well? Thanks :)

[bpostlethwaite]

Great, I think these changes are an improvement. Nice work!

💃

[bpostlethwaite]

What are the Percy visual diffs about though?

[T4rk1n]

There are regressions in the percy diff, the checklist seems broken.

[rmarren1]

I think the tabs screenshots are a bit finicky, I got that Tabs with Graph - clicked tab 1 (graph should not resize) regression before and have also seen it opposite (graph 1 in tab 2).

[valentijnnieman]

@rmarren1 Yeah, the test isn't great for that one. I rewrote it (and am merging it now) in #306

[valentijnnieman]

@T4rk1n @bpostlethwaite Running the code in the test in a Dash app looks fine to me, I have no idea what's up with those Percy diffs.

edit:
and running from master:

[T4rk1n]

@valentijnnieman I confirm the same, working normally in a dash app.

[valentijnnieman]

Perhaps the changes in #312 will give us some better insights into what's going on here.

[T4rk1n]

@valentijnnieman Can we change the webpack config like @bpostlethwaite did here plotly/dash-component-boilerplate#14 (comment) ?