Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Do not import bundled dist version #95

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

Closed
m90 wants to merge 1 commit into plotly:master from m90:entrypoint
Closed

Do not import bundled dist version #95

m90 wants to merge 1 commit into plotly:master from m90:entrypoint

Conversation

m90
Copy link

@m90 m90 commented Aug 14, 2018

Fixes issues with browserify seen in #93 and plotly/plotly.js#2902

Copy link
Contributor

Thanks for looking so hard for solutions on this! Unfortunately we can't do this for Browserify because it makes Webpack much harder to use...

Does #94 not work with Browserify either? I'd be more inclined to merge that one...

Copy link
Author

m90 commented Aug 14, 2018

#94 does not work properly with Browserify either (in my setup). I think the only long-term solution is using derequire as described in plotly/plotly.js#2902 or sitting it out and wait for Browserify 17, hoping they will fix it for us. Alternatively it'd be about properly documenting the limitation and its workaround.

That being said I will hopefully find the time to look into using derequire and if it helps tomorrow, so I'll let you know what happens here.

Copy link
Contributor

OK. Are you not able to use the factory approach and provide your own plotly object?

Copy link
Contributor

(thanks again for persisting with this issue, I appreciate the assistance, having not worked very much with Browserify)

m90 reacted with heart emoji

Copy link
Author

m90 commented Aug 14, 2018

I can use the factory approach just fine, but as stated it's not really what's documented right now, meaning it takes people quite some time to find out about it.

Copy link
Contributor

Well, it's documented here, but I supposed I should make it clearer: https://github.com/plotly/react-plotly.js#customizing-the-plotlyjs-bundle

Copy link
Author

m90 commented Aug 14, 2018

It should mention that if using Browserify it's what needs to be done in any case, not only if you are looking into shaving bytes.

That being said, I'd wait to see if applying derequire on the bundling of plotly itself helps before moving around too much stuff and introducing a Webpack / Browserify binary earlier than it needs to be introduced. Then again that's up to you.

Copy link
Author

m90 commented Aug 15, 2018

I think we can close this now that plotly/plotly.js#2905 has been merged. I wonder what the best way of creating a "pre-version" of react-plotly.js is now, so that we can confirm it's behaving as expected before 1.40.0 is published to npm?

@m90 m90 closed this Aug 15, 2018
Copy link
Contributor

I don't know if we'll be able to test this before 1.40.0 lands, but thanks for your contribution upstream of this project! Hopefully things will go as planned but if they don't, we can always issue a patch release of plotly.js or fix stuff on this side :)

Copy link
Author

m90 commented Aug 16, 2018

So my curiosity just made me look into if the upstream change actually fixes the issue, and I was able to use npm link to make react-plotly use current upstream master. Bundling with Browserify now works as expected and there is no more need for the workaround 🎉

Copy link
Contributor

nicolaskruchten commented Aug 16, 2018 via email

Great, thanks again! But can we still switch to using the -dist bundle as well? I do want to move towards this as that bundle is so much slimmer due to the lack of dependencies.
...
On Thu, Aug 16, 2018 at 10:38 Frederik Ring ***@***.***> wrote: So my curiosity just made me look into if the upstream change actually fixes the issue, and I was able to use npm link <https://docs.npmjs.com/cli/link> to make react-plotly use current upstream master. Bundling with Browserify now works as expected and there is no more need for the workaround 🎉 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#95 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbA6WULc_1wZtJnkcPYQPyyV0-e3Mcks5uRYPwgaJpZM4V8D7w> .

Copy link
Author

m90 commented Aug 16, 2018
edited
Loading

I do not know how the -dist package is being created and published, but in case the plotly.js it ships with is the one that is in the upstream /dist folder it should work just as fine once it's at 1.40.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /