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 intomasterfrom
prod-prop-types
Jul 5, 2019
Merged

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

Conversation

@Marc-Andre-Rivet
Copy link
Copy Markdown
Contributor

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

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
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

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.

Copy link
Copy Markdown
Contributor

@wbrgss wbrgss left a comment

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!

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