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

Feat/covid bundle #737

Merged
merged 3 commits into from
Sep 21, 2020
Merged

Feat/covid bundle #737

merged 3 commits into from
Sep 21, 2020

Conversation

mfshao
Copy link
Collaborator

@mfshao mfshao commented Sep 21, 2020

Separate Covid19 dashboard into another bundle to reduce bundle size.

Supply GEN3_BUNDLE=covid19 as env var to enable covid bundle

There is another ticket to remove Arranger https://ctds-planx.atlassian.net/browse/PXP-6409

New Features

  • Separate Covid19 dashboard into another bundle to reduce bundle size

Deployment changes

  • Add GEN3_BUNDLE=covid19 as portal env var to enable covid bundle

Copy link
Contributor

@frickjack frickjack left a comment

Choose a reason for hiding this comment

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

Hey Mingfei,

I'm approving this, but it would be cool to try to figure out a way that the index page could take some props or something that was some kind of route override. So an abstract index could have default route table:

{ 
    "path1": { exact: true|false, component: <jsx-something /> },
   "path2": ...,
}

, and actual instances of index-page provide overrides somehow:

class CovidIndexPage {
    ...
    routeOverrides = { "path3": { exact, component: <jsx-whatever/>}}
}

where the base class render does something like:

render() {
     const myRoutes = { ...this.defaultRoutes, ...this.overrideRoutes };
     ...
     for (const it of myRoutes.entries()) {
     }

I'm not sure what the best way to do that kind of thing in React is ...

@@ -104,16 +104,23 @@ if (process.env.NODE_ENV !== 'dev' && process.env.NODE_ENV !== 'auto') {

const entry = {
"bundle": ['babel-polyfill', './src/index.jsx'],
"workspaceBundle": ['babel-polyfill', './src/workspaceIndex.jsx']
"workspaceBundle": ['babel-polyfill', './src/workspaceIndex.jsx'],
"covid19Bundle": ['babel-polyfill', './src/covid19Index.jsx']
};

if (process.env.GEN3_BUNDLE === 'workspace') {
Copy link
Contributor

Choose a reason for hiding this comment

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

how would we enable multiple bundles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if you don't specify GEN3_BUNDLE value then it will build all bundles, is that what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

i see, what i was thinking is that we could have GEN3_BUNDLE=covid19,workspace

but then i'm not sure how this entry.bundle stuff would work 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh cool. I don't think we have the need to deploy multiple bundles right now so right now if you spin up a portal pod you would have to choose one flavor only

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fair, then follow up question, why don't we do

if (process.env.GEN3_BUNDLE === 'workspace') {
  entry.bundle = ['babel-polyfill', './src/workspaceIndex.jsx'];
}

and default to only building ./src/index.jsx, instead of deleting entries which looks a bit messy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think for Dockerfile we don't have GEN3_BUNDLE set so it will build all bundles to make sure nothing stops us from building an image. So we can either delete from a full entry array, or adding into it for that case

@mfshao mfshao merged commit a6cea28 into master Sep 21, 2020
@mfshao mfshao deleted the feat/covid-bundle branch September 21, 2020 20:41
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