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

Update build chain for dash 0.32.1 #94

Merged
merged 14 commits into from
Dec 12, 2018
Merged

Update build chain for dash 0.32.1 #94

merged 14 commits into from
Dec 12, 2018

Conversation

pbugnion
Copy link
Member

@pbugnion pbugnion commented Dec 9, 2018

As of release 0.32.1, dash prefers building the Python classes at build time rather than at run time. Dash comes with a new utility, dash-generate-components, that builds the Python classes. This PR refactors the tooling to use this.

Now, running npm install populates dash_bootstrap_components/_components with the Python class files containing the components. These then get built into the source distribution.

I have deliberately diverged from some of the recommendations outlined in issue #93 :

  • The build artefacts are not source controlled. This follows the convention we've adopted in dbc so far.
  • Generated components are segregated to a _components subdirectory, rather than being in the dash_bootstrap_components root. This makes it easier to clearly differentiate Python files that are written manually (e.g. dbc.themes, __init__.py, the upcoming helper to generate a table from a dataframe) from auto-generated source.

At the moment, running npm install shows some tracebacks when react-docgen tries to parse the test files. I think this is an issue that needs to be fixed in dash, not directly in here. I have raised an issue to that effect. This doesn't actually stop us from using the build artefacts, it just makes the build process confusing.

I have released this as 0.2.4-alpha1 to test it and everything seems to be working. Nevertheless, since this is a fairly tricky change, it'd be great to:

  • check that you can actually run npm i (and then maybe run the documentation or some other app)
  • check that you can install and run something off 0.2.4-alpha1 with pip install dash-bootstrap-components==0.2.4-alpha1.

Addresses issue #93.

@pbugnion pbugnion changed the title [WIP] Update build chain for dash 0.32.1 Update build chain for dash 0.32.1 Dec 9, 2018
"build-dist": "npm run clean:py && npm run build:py && webpack --config=webpack.config.dist.js",
"clean:py": "mkdirp dash_bootstrap_components/_components && rimraf dash_bootstrap_components/_components",
"demo": "webpack-dev-server --hot --inline --port=8888 --config=webpack.config.demo.js",
"build:py": "mkdirp dash_bootstrap_components/_components && dash-generate-components ./src/components dash_bootstrap_components/_components && move-cli dash_bootstrap_components/_components/_imports_.py dash_bootstrap_components/_components/__init__.py",
Copy link
Collaborator

@tcbegley tcbegley Dec 10, 2018

Choose a reason for hiding this comment

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

Shall we add something like black dash_bootstrap_components/_components to this? Otherwise the generated Python files are going to fail the build once they get added to version control.

Actually ignore this, I think we're not keeping these under version control right? We're just generating them for inclusion in the distribution?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we don't keep the generated sources under version control (unlike, for instance, dash-core-components).

While we could still reformat them with black, I'd vote against because:

  • it makes building a little bit longer for no benefit
  • it adds more point of failures in the build chain.

@tcbegley
Copy link
Collaborator

Looks great @pbugnion! Thanks for looking into this and setting everything up.

I have testing building, running the demo, and installing 0.2.4-alpha1 and all worked for me.

I made a comment inline, but have since thought that actually it's not relevant, and this is all fine as is.

@tcbegley tcbegley self-requested a review December 10, 2018 11:30
@pbugnion pbugnion merged commit b39ffe2 into master Dec 12, 2018
@pbugnion pbugnion deleted the update-build-chain branch December 12, 2018 07:33
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

2 participants