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

wavesurfer-react: V3 #72

Closed
ShiiRochi opened this issue Oct 30, 2023 · 18 comments · Fixed by #73
Closed

wavesurfer-react: V3 #72

ShiiRochi opened this issue Oct 30, 2023 · 18 comments · Fixed by #73
Assignees
Labels
enhancement New feature or request

Comments

@ShiiRochi
Copy link
Owner

ShiiRochi commented Oct 30, 2023

wavesurfer.js version below 7 will be supported in wavesurfer-react version 2 which can be found now in branch v2

@ShiiRochi ShiiRochi pinned this issue Oct 30, 2023
@ShiiRochi
Copy link
Owner Author

Within this task all changes should be made to make wavesurfer-react to support wavesurfer.js of version 7 and above

@ShiiRochi
Copy link
Owner Author

If Region component is rendered from the start wavesurfer applies incorrect start and end properties there, i.e. instead of use passed ones it will use 0 and 0 if audio is not uploaded yet

@ShiiRochi
Copy link
Owner Author

Also, only one instance of a plugin should be initiated during life-cycle of a component

@ShiiRochi
Copy link
Owner Author

According to https://github.com/katspaugh/wavesurfer.js/blob/main/examples/regions.js, regions can be created right after decoding event s emiitted

@ShiiRochi
Copy link
Owner Author

I also think about a way to create a component like PluginController, that will be used like:

const example = (
  <WaveSurfer {..._props}>
    <WaveForm {...formProps}>
      <PluginBridge Plugin={RegionsPlugin}>
        {regions.map(reg => <Region {...reg} />)}
      </PluginBridge>
    </WaveForm>
  </WaveSurfer>
);

Aim of plugin bridge is to control plugin life-cycle, i.e. currently plugin is registered via wavesurfer instance, but is destroyed by itself, wavesurfer just listens to 'destroy' event and removes plugin that emitted it.

@ShiiRochi
Copy link
Owner Author

ShiiRochi commented Nov 5, 2023

I also think about a way to create a component like PluginController, that will be used like:

const example = (
  <WaveSurfer {..._props}>
    <WaveForm {...formProps}>
      <PluginBridge Plugin={RegionsPlugin}>
        {regions.map(reg => <Region {...reg} />)}
      </PluginBridge>
    </WaveForm>
  </WaveSurfer>
);

Aim of plugin bridge is to control plugin life-cycle, i.e. currently plugin is registered via wavesurfer instance, but is destroyed by itself, wavesurfer just listens to 'destroy' event and removes plugin that emitted it.

This way it would be easier to controll plugins used to build UI of wavesurfer. It would be easier to control same instances, i.e. avoiding PluginInstance recreation on each render on each plugin list creation.

For example, the following code shows how to use more than one plugin at the same time with the way proposed above:

const example = (
  <WaveSurfer {..._props}>
    <WaveForm {...formProps}>
      <PluginBridge Plugin={RegionsPlugin} {...regionsPluginProps}>
        {regions.map(reg => <Region {...reg} />)}
      </PluginBridge>
      <PluginBridge Plugin={TimelinePlugin} {...timelinePluginProps} />
      <PluginBridge Plugin={MyAwesomePlugin} creator="createMyAwesome" {...timelinePluginProps} />
    </WaveForm>
    <div id="timeline" />
  </WaveSurfer>
);

@ShiiRochi
Copy link
Owner Author

This way is good enough, but what if some component will depend on both Regions and Timeline plugin...it should not take them from wavesurfer instance due to the fact that wavesurfer object in useState is not updated when something is destroyed.

@ShiiRochi
Copy link
Owner Author

Thus...plugins list will be still in use for a while until better way to control plugins is found

@ShiiRochi
Copy link
Owner Author

Also, there could be used two timelines at the same time, thus instanceof check alone is not suitable as solution to prevent plugin instance recreation.

It seems, additional field should be used along with instanceof check to differentiate different plugins...or...such field is required if more than one instance of one plugin is created.

@ShiiRochi
Copy link
Owner Author

Found out a bug in wavesurfer.js - katspaugh/wavesurfer.js#3314, will continue to develop version 3 as like as this bug is already fixed.

@ShiiRochi
Copy link
Owner Author

Found out a bug in wavesurfer.js - katspaugh/wavesurfer.js#3314, will continue to develop version 3 as like as this bug is already fixed.

Task was closed as not planned however I will try to investigate it further, because it should not work this way.

@ShiiRochi
Copy link
Owner Author

Made extra investigation during which I created simple vanilla JS example replicating the same wrong behavior (link to the comment below): katspaugh/wavesurfer.js#3314 (comment)

@ShiiRochi
Copy link
Owner Author

ShiiRochi commented Nov 6, 2023

I will not recreate bug issue, but provide users of the package with a temporal solution to this bug of original package

@ShiiRochi ShiiRochi added the enhancement New feature or request label Nov 6, 2023
@ShiiRochi ShiiRochi linked a pull request Nov 6, 2023 that will close this issue
@ShiiRochi
Copy link
Owner Author

Found out a bug in wavesurfer.js - katspaugh/wavesurfer.js#3314, will continue to develop version 3 as like as this bug is already fixed.

Task was closed as not planned however I will try to investigate it further, because it should not work this way.

Task was reopened

@ShiiRochi
Copy link
Owner Author

ShiiRochi commented Nov 7, 2023

Markers plugins was deleted in wavesurferjs, instead Regions with content should be used.

Due to this fact I may leave Marker only by using Region component under the hood.

@ShiiRochi
Copy link
Owner Author

Changed the way how plugins are registered.

Added a key property in PluginType, so now wavesurfer-react is taught how to track record of already registered in wavesurfer instance plugins.

@ShiiRochi
Copy link
Owner Author

Adjusted workflows to also include build step as a crucial part of contribution. Each contribution should keep the package's build capability.

@ShiiRochi
Copy link
Owner Author

ShiiRochi commented Nov 28, 2023

For a while will keep demo folder within the repo, but after a period of time will need to make a separate repository to handle development demo. Also need to find out how to deploy demo to some sort of hosting...maybe Cloudflare will suit for my current needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant