-
-
Notifications
You must be signed in to change notification settings - Fork 49
Conversation
Should there be a demo for this repo?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 Just not sure about the benefits of adding prettier to auto generated code.
.circleci/config.yml
Outdated
paths: | ||
- "venv" | ||
- "node_modules" | ||
|
||
- run: | ||
name: prettier --list-different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why prettier ? It's auto generated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this would probably just get annoying. I'll remove it
scripts/publish.js
Outdated
throw new Error('\nIt looks like there are uncommitted changes! Aborting until these changes have been resolved.\n'); | ||
} else { | ||
execSh([ | ||
'npm publish --otp', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think you can remove the --otp, it will asks if need be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, wasn't sure what that did, I just use npm publish
then allow it to 401
and ask for the OTP. This was copied from the dash-core-components
repo, so this happens there as well.
99f1733
to
d9a0b6a
Compare
package.json
Outdated
"uninstall-local": "pip uninstall dash-html-components -y", | ||
"prepublish": "npm run clean && npm run generate-components && npm test && npm run build:js && npm run build:js-dev && npm run build:py", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we condense this scripts by using build:all
?
|
@T4rk1n (reminder) |
"clean-src": "rm -rf src/* && mkdir -p src/components", | ||
"clean": "npm run clean-lib && npm run clean-src", | ||
"copy-lib": "cp lib/* dash_html_components", | ||
"clean": "rm -rf src/* && mkdir -p src/components", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work on windows natively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, the clean
command and publish script doesn't work on windows.
publish script is same as in DCC, so that problem will exist there too. |
Not sure the best way to solve this, some options I can think of:
|
"uninstall-local": "pip uninstall dash-html-components -y", | ||
"prepublish": "npm run clean && npm run generate-components && npm run build:all", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove prepublish entirely and use the new npm methods it warns about instead. This will throw when you install on windows.
Let's not fix those platform issues for now, clean/generate will run under cygwin/gitbash. Windows dev can just run the commands to publish separately, that's what I do. |
👍 |
Update tooling like in plotly/dash-core-components#299
Also, this moved
requirements.txt -> .circleci/dev/dev-requirements.txt
and makes that file minimal, like in this PR: plotly/dash#439