Skip to content

Updating scripts to match dash-core-components #20

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

Open
valentijnnieman opened this issue Oct 16, 2018 · 6 comments
Open

Updating scripts to match dash-core-components #20

valentijnnieman opened this issue Oct 16, 2018 · 6 comments

Comments

@valentijnnieman
Copy link

We should update the scripts in package.json the same way as we did for dash-core-components. However, I don't think we should force prettier on the users, since some people are not a fan (although I noticed it's pretty much set up already in this repo, except there's no prettier package in package.json's dependencies. If we do want it, we could use the format and format:test scripts). We should copy over the publish script however, or perhaps add it to a scripts repo as mentioned in #17. We should also copy the build:watch script, which rebuilds the bundle on src/ files changes, the 'lint and test scripts are also nice to have here.

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 16, 2018

I personally hates prettier and do not want to see it here forced on users. I think the formatted code (with the dcc configs) is actually uglier, less readable (if you already have sane line length) and harder to refactor IMO (splitting lines makes them harder to move around with my IDE ctrl+shift selection).

As for the publish script, what does it saves ? like one command line instead of three, I don't think it's that useful. Also, you don't have to publish on npm if you don't want to, so the script would have to be adapted to take that variable into consideration.

The build:watch , lint, test scripts should be added.

@valentijnnieman
Copy link
Author

Like I said I agree that prettier shouldn't be forced (so perhaps we should remove the .prettierrc and other prettier config?) the publish script is handy IMO if you've never published before and just want to run a single command without having to read up on how to use twine etc. I was also under the impression that the JS source code was downloaded by Dash (or dash-renderer?) from NPM so every component would need it's JS code published on NPM?

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 16, 2018

They don't need to be uploaded to NPM, I discovered that you only need to remove the external_url from the _js_dist to force dash to serve locally, even if app.scripts.config.serve_locally=False. plotly/dash#405

So I added a publish_on_npm cookiecutter variable to disable it if false.

@valentijnnieman
Copy link
Author

Oh nice! In any case, it could still be useful to have a simple script that does twine upload dist/component_version_0.0.1.tar.gz or something for you, which is what that publish script in DCC does. It might not be all that useful to you, but for newcomers it could be useful.

@valentijnnieman
Copy link
Author

Just linking to plotly/dash-core-components#299 (comment) where we discussed making the same improvements to these repo's too, in case that wasn't clear. Haven't had time to make those improvements yet, so future me if you're reading this, this is on you!

@valentijnnieman
Copy link
Author

But seriously if anyone else feels like picking this up, that would be great!

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

2 participants