Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Handle external 'prop-types' correctly in prod build#488

Merged
Marc-Andre-Rivet merged 12 commits into
masterfrom
prod-prop-types
Jul 5, 2019
Merged

Handle external 'prop-types' correctly in prod build#488
Marc-Andre-Rivet merged 12 commits into
masterfrom
prod-prop-types

Conversation

@Marc-Andre-Rivet

@Marc-Andre-Rivet Marc-Andre-Rivet commented Jul 3, 2019

Copy link
Copy Markdown
Contributor

This PR correctly externalizes prop-types for usage as a library. It also exposes the library as a prop on window to be consistent with other Dash libraries.

Would normally not update the build as part of a PR but this is included in another project through package.json and makes everything easier to handle on the other end.

"dash-table": "github:plotly/dash-table#prod-prop-types"

@Marc-Andre-Rivet
Marc-Andre-Rivet requested a review from wbrgss July 3, 2019 13:51
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-488 July 3, 2019 14:04 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-488 July 3, 2019 14:05 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-488 July 3, 2019 14:15 Inactive
@Marc-Andre-Rivet
Marc-Andre-Rivet marked this pull request as ready for review July 3, 2019 14:17
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-488 July 3, 2019 19:23 Inactive
@Marc-Andre-Rivet
Marc-Andre-Rivet requested a review from chriddyp July 3, 2019 20:21
Comment thread .config/webpack/base.js
root: 'ReactDOM'
},
react: 'React',
'react-dom': 'ReactDOM',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This can be simplified as it's target is window now

Comment thread .config/webpack/base.js
filename: '[name].js',
library: dashLibraryName,
libraryTarget: 'umd'
libraryTarget: 'window'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Loaded unto window.dash_table in the browser

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will changing how this is exposed have any side-effects for non-Dash users? Could you imagine anyone using RequireJS or anything else that expects a umd in their JavaScript project? I suppose, obviously, we wouldn't expect Node.js users (without a window global namespace) to be using this project. I'm also not sure if there's any webpack magic that makes this a non-issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should not have an impact.

@wbrgss wbrgss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! I believe this will fix "issues" like this right?

image

LGTM overall but I'd like some clarification on the libraryTarget below if you could shed some light on my webpack understanding!

Comment thread .config/webpack/base.js
filename: '[name].js',
library: dashLibraryName,
libraryTarget: 'umd'
libraryTarget: 'window'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will changing how this is exposed have any side-effects for non-Dash users? Could you imagine anyone using RequireJS or anything else that expects a umd in their JavaScript project? I suppose, obviously, we wouldn't expect Node.js users (without a window global namespace) to be using this project. I'm also not sure if there's any webpack magic that makes this a non-issue.

@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-488 July 4, 2019 20:24 Inactive
@Marc-Andre-Rivet

Copy link
Copy Markdown
Contributor Author

@wbrgss Yes. It will fix the missing prop types dep

@Marc-Andre-Rivet

Copy link
Copy Markdown
Contributor Author

Adding a changelog entry and will merge afterwards.

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.

3 participants