Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Update tooling #79

Merged
merged 17 commits into from
Dec 12, 2018
Merged

Update tooling #79

merged 17 commits into from
Dec 12, 2018

Conversation

rmarren1
Copy link
Contributor

@rmarren1 rmarren1 commented Nov 6, 2018

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

@rmarren1
Copy link
Contributor Author

rmarren1 commented Nov 6, 2018

Should there be a demo for this repo?
npm run start does not work.

webpack.server.config.js points to ./src/demo/index.js, but that file does not exist.

Copy link
Contributor

@T4rk1n T4rk1n left a 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.

paths:
- "venv"
- "node_modules"

- run:
name: prettier --list-different
Copy link
Contributor

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.

Copy link
Contributor Author

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

throw new Error('\nIt looks like there are uncommitted changes! Aborting until these changes have been resolved.\n');
} else {
execSh([
'npm publish --otp',
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@rmarren1
Copy link
Contributor Author

rmarren1 commented Dec 1, 2018

@T4rk1n care to take another look?

The CI error in python3.7 is a E104 so it should fix itself with another run; however, it doesn't look like I have permissions to re-trigger builds for this repo (I do for other ones though). @chriddyp can I get permissions to do that in the future?

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",
Copy link
Contributor

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 ?

@rmarren1
Copy link
Contributor Author

rmarren1 commented Dec 7, 2018

@T4rk1n

  • Made that simplification to prepublish
  • Removed some more scripts that are now obselete (after removing archetype)
  • Updated README with more up to data instructions
  • Re-built the whole thing -- for some reason this created a bunch of diffs, but I am not sure what changed. It looks like that issue with ^M, but I thought that was fixed

@rmarren1
Copy link
Contributor Author

@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",
Copy link
Contributor

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.

Copy link
Contributor

@T4rk1n T4rk1n left a 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.

@rmarren1
Copy link
Contributor Author

publish script is same as in DCC, so that problem will exist there too.

@rmarren1
Copy link
Contributor Author

Not sure the best way to solve this, some options I can think of:

  • Write out own script in a higher level language (e.g. python) that takes care of compatibility
  • have a clean and clean:windows command simultaneously

"uninstall-local": "pip uninstall dash-html-components -y",
"prepublish": "npm run clean && npm run generate-components && npm run build:all",
Copy link
Contributor

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.

@T4rk1n
Copy link
Contributor

T4rk1n commented Dec 12, 2018

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.

@rmarren1 rmarren1 merged commit fec4272 into master Dec 12, 2018
@rmarren1
Copy link
Contributor Author

👍

@rmarren1 rmarren1 deleted the update-tooling branch December 12, 2018 02:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants