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

Bundle components separately (based on exports in the inner index.js files) #513

Merged
merged 8 commits into from Apr 22, 2020

Conversation

keller-mark
Copy link
Member

@keller-mark keller-mark commented Apr 21, 2020

This pull request updates the bundling scripts so that they output the following files:

build-lib/
    es/
        development/
            cell-tooltip.js
            channels.js
            factors.js
            genes.js
            heatmap.js
            index.js    <--- from src/index.js
            scatterplot.js
            sets.js
            sourcepublisher.js
            spatial.js
            status.js
            static/css/index.css
        production/
            cell-tooltip.min.js
            channels.min.js
            factors.min.js
            genes.min.js
            heatmap.min.js
            index.min.js    <--- from src/index.js
            scatterplot.min.js
            sets.min.js
            sourcepublisher.min.js
            spatial.min.js
            status.min.js
            static/css/index.css
    umd/
        development/
            (same filenames as es/development/ above)
        production/
            (same filenames as es/production/ above)

build-demo/ (same output as before)

Each .js file corresponds to the "inner" index.js files located at src/components/{component}/index.js with the exception of the build index.js which comes from src/index.js.

These are now explicitly listed in scripts/paths.js here. Should this listing be automated?

I also split the webpack config into:

  • webpack.config-lib.js
  • webpack.config-demo.js
  • webpack.config-common.js

and in doing so removed most of the dependence on global process.env constants by instead wrapping things in functions that take those values as arguments. I copied the original comments when moving code into webpack.config-common.js and wrapping the code in functions.

However, this now means that our bundle scripts now deviate much more from create-react-app.

I think that it would be a bit nicer if the build-lib directory just contained es/ and umd/ and then the development and production files were differentiated by .js and .min.js but I could not get this to work. Production and development builds currently use separate webpack configs, and the scripts/utils.js Line 153 runs fs.emptyDirSync(buildDir); before building. Removing this line caused an infinite loop for some reason, but maybe someone has an idea for how this could be fixed?

You can test the library build with npm run build-lib, the demo build with npm run build-demo, and the demo dev server build with npm run start.

Fixes #379

Copy link
Collaborator

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Nothing too major - would be curious on thoughts about the es minifying. Also, maybe you could update the README.md with a code snippet of how importing works? Looks really good otherwise - this could be something that would be cool to part out in its own repo eventually, even! Having human-readable webpack config that does complicated stuff is no small feat!

package.json Show resolved Hide resolved
scripts/paths.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@mccalluc mccalluc left a comment

Choose a reason for hiding this comment

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

Looks good to me. Does this have any relationship to #504, or is that separate?

And is the motivation here the reuse needs articulated by Glasgow, or something else?

const utils = require('./utils');
const paths = require('./paths');
const configFactory = require('./webpack.config-demo');
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe keep .config as the extension?

Copy link
Member Author

Choose a reason for hiding this comment

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

The names will collide between demo, lib, and common then. Can you propose a new way to name the different files?

scripts/build-demo.js Outdated Show resolved Hide resolved
scripts/webpack.config-demo.js Show resolved Hide resolved
scripts/webpack.config-common.js Show resolved Hide resolved
scripts/webpack.config-lib.js Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
@keller-mark
Copy link
Member Author

This is unrelated to #504, and I added more info there so that it stays organized under the issue.

The motivation for this was mostly from the Glasgow team. But also, after I made the original bundling PR, I realized that the Vitessce bundle used to include more output files (these can be browsed on unpkg here https://unpkg.com/browse/vitessce@0.0.24/es/) and that in general it seems like a good idea to provide more fine-grained imports for consumers of Vitessce.

@keller-mark
Copy link
Member Author

keller-mark commented Apr 22, 2020

The dev server (npm run start) and all builds now work in Node 13, so I have updated the readme and travis script to reflect this. https://travis-ci.org/github/hubmapconsortium/vitessce/builds/678228722 I have also added a code snippet to the readme, thanks for the suggestion @ilan-gold

Node 14 came out yesterday(/today?). Using NVM I tested and there are understandably still some dependencies that are not ready yet. In particular, node-sass: sass/node-sass#2895

@keller-mark keller-mark merged commit 3111c65 into master Apr 22, 2020
@keller-mark keller-mark deleted the keller-mark/bundle-individual-components branch April 22, 2020 16:12
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.

Upgrade to Node 12
3 participants