bhofmei / jbplugin-screenshot

JBrowse plugin that adds button which uses phantomJS to take browser screenshot
Other
10 stars 3 forks source link

Simplify main module #12

Closed cmdcolin closed 6 years ago

cmdcolin commented 6 years ago

Hello

I was wondering if you would consider this PR to simplify the main.js of this module. The current module does not build with 1.13+ versions of jbrowse due to webpack, but with this modification it does work.

Do you think it would be possible to consider this change? If the additional modules that are deleted via this PR are necessary in other contexts (e.g. I know in previous jbrowse versions, if the plugin uses a package that jbrowse itself doesn't use, then pre-packaging those modules for the plugin helps) but maybe that step of pre-packaging can be in the built release artifacts?

Let me know what you think

bhofmei commented 6 years ago

Since it doesn’t work with v1.13+, I’ll definitely consider the PR. I personally haven’t been able to switch over to JBrowse v1.13.1 because it requires Node.js v7+ and I have a more pressing project which uses Node.js v6 (I want to bring this project to the current version of Node.js but cannot at this time). Also the publication for this plugin is under review and I’m hesitant to make any major changes during the review process.

Those extra modules are ones needed by the plugin that are not part of JBrowse. The only way to get it to work was to include it in the plugin itself.

Once I’m able to switch to a newer version of Node, I’ll try to find a better solution.

On Apr 18, 2018, at 2:38 PM, Colin Diesh notifications@github.com wrote:

Hello

I was wondering if you would consider this PR to simplify the main.js of this module. The current module does not build with 1.13+ versions of jbrowse due to webpack, but with this modification it does work.

Do you think it would be possible to consider this change? If the additional modules that are deleted via this PR are necessary in other contexts (e.g. I know in previous jbrowse versions, if the plugin uses a package that jbrowse itself doesn't use, then pre-packaging those modules for the plugin helps) but maybe that step of pre-packaging can be in the built release artifacts?

Let me know what you think

You can view, comment on, or merge this pull request online at:

https://github.com/bhofmei/jbplugin-screenshot/pull/12 https://github.com/bhofmei/jbplugin-screenshot/pull/12 Commit Summary

Simplify main module File Changes

M js/main.js https://github.com/bhofmei/jbplugin-screenshot/pull/12/files#diff-0 (1803) Patch Links:

https://github.com/bhofmei/jbplugin-screenshot/pull/12.patch https://github.com/bhofmei/jbplugin-screenshot/pull/12.patch https://github.com/bhofmei/jbplugin-screenshot/pull/12.diff https://github.com/bhofmei/jbplugin-screenshot/pull/12.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bhofmei/jbplugin-screenshot/pull/12, or mute the thread https://github.com/notifications/unsubscribe-auth/APWCOiGQAE8rfqbvDf-z_vug-ahWGKFdks5tp4gkgaJpZM4TakYy.

cmdcolin commented 6 years ago

That's ok, I was just asked about it by @scottcain and came up with this possible solution. Good luck on the submitted work that sounds awesome!

cmdcolin commented 6 years ago

Looks like this is live on the main branch now :) I'll close for now