Skip to content

Conversation

mkcor
Copy link
Contributor

@mkcor mkcor commented Apr 4, 2019

Closes #300.

About

We wanted to ensure that dash_bio/bundle.js builds successfully. This is now done, with the changes submitted here.

Description of changes

I've added command npm run build:js to the "node" job, which runs in a node.js environment.

Ultimately, we may want to address the following warning:
size_limit_warning

Before merging

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-304 April 4, 2019 15:35 Inactive
@mkcor mkcor changed the title Add pkgbuild ci Build actual dash_bio node.js package in CI build. Apr 4, 2019
@mkcor mkcor requested review from VeraZab and shammamah-zz April 4, 2019 15:36
shammamah-zz
shammamah-zz previously approved these changes Apr 4, 2019
@mkcor
Copy link
Contributor Author

mkcor commented Apr 4, 2019

The Python classes are built via npm run build:py -- I wonder how/where we could run this command...

@shammamah-zz
Copy link
Contributor

@VeraZab Is there anything that we can do about the size of the bundle? We're already using the minified code.

@shammamah-zz
Copy link
Contributor

@mkcor npm run build:all runs build:js and build:py -- so that's the only one that we need to run :)

@shammamah-zz shammamah-zz dismissed their stale review April 4, 2019 15:39

Misread the script name.

@mkcor
Copy link
Contributor Author

mkcor commented Apr 4, 2019

@VeraZab Is there anything that we can do about the size of the bundle? We're already using the minified code.

Well, first we should study https://webpack.js.org/guides/code-splitting/ in depth--has any one of us already done so? I haven't.

@shammamah-zz
Copy link
Contributor

Right, that makes sense. Is there a way to install the Python requirements before running the node job? Here

workflows:

I'm seeing that you can define jobs and then run them in a specific order: https://circleci.com/docs/2.0/workflows/

So we can just create a new job in our CircleCI config that installs dash (that should be the only package used to build the Python components), and run that before we run npm run build:all.

@mkcor
Copy link
Contributor Author

mkcor commented Apr 4, 2019

@mkcor npm run build:all runs build:js and build:py -- so that's the only one that we need to run :)

As is, we have separate node and python envs, so we can't run npm run build:js & npm run build:py at once.

Misread the script name.

When reviewing a PR, it's useful to look at the Commits tab, not just the diff (Files tab).

@mkcor
Copy link
Contributor Author

mkcor commented Apr 4, 2019

@shammamah So we can just create a new job in our CircleCI config that installs dash

No, each job has its own docker image, as you can see. So if you want to use a dependency that was just installed (dash in our case), these two commands have to run within the same job.

I'm happy to install dash before running npm run build:py but I think we should keep the two commands (npm run build:js & npm run build:py) so the sequence is explicit.

@shammamah-zz
Copy link
Contributor

Sure, I think that makes the most sense.

@mkcor
Copy link
Contributor Author

mkcor commented Apr 4, 2019

@shammamah wait, I think it's really not straightforward to run Python code in this Node environment... We are now using a Docker image that's readily available:

- image: circleci/node:11.10.1

We would need a different (custom, Python-equipped) one, should we run the following Python code:

import dash; dash.development.component_loader.generate_classes('dash_bio', 'dash_bio/metadata.json')

as done by npm run build:py:

"build:py": "node ./extract-meta src/lib/components > dash_bio/metadata.json && copyfiles package.json dash_bio && python -c \"import dash; dash.development.component_loader.generate_classes('dash_bio', 'dash_bio/metadata.json')\"",

I'm hesitating between two options:

  • creating a dedicated Docker image suitable for running npm run build:py;
  • breaking down the various commands contained in npm run build:py (doable by resorting to store_artifacts).

Gets me thinking a lot about the node/python coupling...

@shammamah-zz
Copy link
Contributor

creating a dedicated Docker image suitable for running npm run build:py;

I'd choose this, since it represents a well-defined "step" in our build process.

@VeraZab
Copy link
Contributor

VeraZab commented Apr 4, 2019

@mkcor @shammamah this looks like a reasonable change to me.
I feel like you guys know better at this point, since I haven't kept updated about the latest evolvements of the repo.

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-304 April 11, 2019 12:59 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-304 April 11, 2019 13:11 Inactive
@mkcor
Copy link
Contributor Author

mkcor commented Apr 11, 2019

I went for a Node.js variant Docker image, including our own specific version of Node.js / NPM as exemplified here: https://circleci.com/docs/2.0/circleci-images/#best-practices
This way, we can compile the components into Python right after building the Node.js package (9c9a4bd) and after installing dash (7d4a1a2).

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-304 April 11, 2019 13:18 Inactive
@mkcor
Copy link
Contributor Author

mkcor commented Apr 11, 2019

I thought caching the dash dependency wasn't worthwhile, hence 18010a7.

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-304 April 11, 2019 13:29 Inactive
@mkcor
Copy link
Contributor Author

mkcor commented Apr 11, 2019

Oh, and I probably need to cherry-pick this commit 1b32450 :)

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-304 April 11, 2019 13:38 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-304 April 11, 2019 14:09 Inactive
Co-Authored-By: mkcor <marianne.corvellec@ens-lyon.org>
@mkcor mkcor merged commit c02c941 into master Apr 11, 2019
@mkcor mkcor deleted the add-pkgbuild-ci branch April 11, 2019 14:39
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

Successfully merging this pull request may close these issues.

3 participants