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

Releasing 2.0.0 #1092

Closed
mspae opened this issue May 14, 2017 · 27 comments
Closed

Releasing 2.0.0 #1092

mspae opened this issue May 14, 2017 · 27 comments
Milestone

Comments

@mspae
Copy link
Contributor

mspae commented May 14, 2017

There has been an extensive refactor of wavesurfer within the last half year which resulted in a version which includes a lot of new features (namely a dynamic plugin system) but also some breaking changes. We have decided to release it as a version two. Changes to either branch will be ported to each other (especially bug fixes of course).

This issue is intended as a place for general discussion and feedback regarding the v2 release.

Edit 1: Added change I forgot to mention: MultiCanvas is the default and only inbuilt renderer

The version is mostly stable and functional. Due to the nature of the refactor (an integrated plugin system for adding, initialising and destroying of plugin instances) all plugins are somehow affected. Some plugins are still buggy when used with the dynamic plugin API. Conventional use cases (i.e. cases where you just want to create an instance with some plugins – no dynamic adding or removing – should work just fine) (Below in the plugin API examples, this means 1. should already work, 2. and 3. don't yet always work in all situations and for all plugins)

Installing the release

To get the current beta release (at the time of writing) do npm install wavesurfer.js@2.0.0-beta01

You can find the beta release(s) on the releases page or by running npm show wavesurfer.js | grep beta

Developing

To work on the new branch checkout the next branch, do npm install and then npm start – The library directory is exposed under http://localhost:8080

Generate the documention by running npm run doc (see below) – Tests can be run by calling npm run test

There are still lots of things which need doing. We welcome and are thankful for any help we can get. Please see right at the bottom of this post how you can help us.

Changes

(See a full list of all merged PRs for v2)

  • ES6 Syntax: Refactored all code to use the new ES6/ES2015 syntax (especially arrow functions, classes, spread operators etc.)
  • New build system: Webpack is being used as a build system to compile the code. It can be imported like this:
// EITHER - accessing modules with <script> tags
var WaveSurfer = window.WaveSurfer;
var TimelinePlugin = window.WaveSurfer.timeline;
var MinimapPlugin = window.WaveSurfer.minimap;

// OR - importing as es6 module
import WaveSurfer from 'wavesurfer.js';
import TimelinePlugin from 'wavesurfer.js/dist/plugin/wavesurfer.timeline.min.js';
import MinimapPlugin from 'wavesurfer.js/dist/plugin/wavesurfer.minimap.min.js';

// OR - importing as require.js/commonjs modules
var WaveSurfer = require('wavesurfer.js');
var TimelinePlugin = require('wavesurfer.js/dist/plugin/wavesurfer.timeline.min.js');
var MinimapPlugin = require('wavesurfer.js/dist/plugin/wavesurfer.minimap.min.js');
  • Documentation: Added loads of jsdoc style documentation tags and the esdoc documentation generator (the documentation needs to be also made public, currently it is generated into ./doc by calling npm run doc) – The idea being that it is easier to keep documentation up to date easier than having to always edit HTML code in the gh-pages branch.
  • New plugin API: Previously all plugins, backends and drawers relied on the window.WaveSurfer global variable. This made it difficult to work with module bundlers (You had to expose the WaveSurfer object, which means it's not really modular) – To fix this plugins now all follow a common format which is used by wavesurfer core to register the plugins correctly. (see an example of a plugin class at the bottom of this post)

There are three ways to add a plugin:

  1. Adding and initialising plugin immediately (this will do in 99% of the cases):
var wavesurfer = WaveSurfer.create({
    container: '#waveform',
    waveColor: 'violet',
    plugins: [
        TimelinePlugin.create({
            container: '#wave-timeline'
        })
    ]
});
  1. Adding a plugin immediately and initialising it dynamically:
var wavesurfer = WaveSurfer.create({
    container: '#waveform',
    plugins: [
        TimelinePlugin.create({
            container: '#wave-timeline',
            deferInit: true // stop the plugin from initialising immediately
        })
    ]
});
wavesurfer.initPlugin('timeline');
  1. Adding and initialising a plugin dynamically:
var wavesurfer = WaveSurfer.create({
    container: '#waveform',
    waveColor: 'violet'
});
// adding and initialising a plugin after initialisation
wavesurfer.addPlugin(TimelinePlugin.create({
    container: '#wave-timeline',
})).initPlugin('timeline');

Also a plugin can be destroyed like this:

wavesurfer.destroyPlugin('timeline');
  • New HTML init script: The html-init.js was extended to allow more complicated initialisation options (including autoloading of plugin code) – see Next: Html Init module #946 – Example usage:
<wavesurfer
  data-url="../media/demo.wav"
  data-plugins="minimap,timeline"
  data-minimap-height="30"
  data-minimap-wave-color="#ddd"
  data-minimap-progress-color="#999"
  data-timeline-font-size="13px"
  data-timeline-container="#timeline"
>
</wavesurfer>
<div id="timeline"></div>
  • MultiCanvas is the default renderer: It supports the functionality of the default renderer but is not constrained by maximum canvas sizes.

Plugin class format

All plugins now follow a common format and their initialisation is handled by wavesurfer core. It is no longer necessary to manually initialise them.

export default class MyAwesomePlugin {
    /**
     * MyAwesome plugin definition factory
     *
     * This function must be used to create a plugin definition which can be
     * used by wavesurfer to correctly instantiate the plugin.
     *
     * @param  {MyAwesomePluginParams} params parameters use to initialise the
     * plugin
     * @return {PluginDefinition} an object representing the plugin
     */
    static create(params) {
        return {
            name: 'myawesome',
            deferInit: params && params.deferInit ? params.deferInit : false,
            params: params,
            staticProps: {
                staticMethod() {
                    // this will be added to the wavesurfer instance and can then be called
                    // wavesurfer.staticMethod();
                }
            },
            instance: MyAwesomePlugin
        };
    }

    constructor(params, ws) {
      // instantiate the plugin
    }

    init() {
      // start doing something
    }

    destroy() {
      // stop doing something
    }
}

Things left to do!

  • Wavesurfer.js (including plugins) is a fairly complex piece of software, that's why it's very important to have lots of people test it and file bugs!
  • Although the core library has been refactored and documented very thoroughly the changes in the plugin code have been very minimal. They still lack jsdoc style comments* and can still be optimised/need to be debugged for use by the dynamic plugin API (*This would be very useful since that would make them appear in the generated documentation.)
  • There are still some memory leaks (which is also true of the current version) – identifying and fixing them would make the whole library stabler.
  • In general a lot of issues are documentation issues in the sense that they crop up because stuff is undocumented. Documenting is a proactive way of stopping those issues from every cropping up. Correcting and writing documentation, writing tutorials and how tos and so on would be very cool!

random cat gif

@katspaugh
Copy link
Owner

katspaugh commented Jul 16, 2017

@mspae I've force-pushed next into the master branch, so that we can accept PRs into the next branch by default. Should've done it long ago.

How do we go about backporting the PRs that got merged into the old master?

Edit: apparently, this diff can serve as a rough overview of things that need to be merged into the next branch.

@mspae
Copy link
Contributor Author

mspae commented Aug 10, 2017

Hey, sorry for the long absence. Unfortunately I'm pretty busy with work right now, so I don't have that much time to commit to working on wavesurfer core fully at this time.

I think concerning the unported changes (= stuff from es5 which isn't in es6 yet) there is nothing for it but to grind them down until we are up to date. But maybe we need to organise this in a clever way to maximise the effect of our time.

I just looked through the diff you mentioned and I think the commits can be categorised like this:

  • Stuff that has already been ported but was put into seperate commits/stuff that isn't relevant for es6 branch
  • New plugin & features: namely the elan-wave-segment plugin
  • Bugs and optimisations: Basically everything that isn't covered by the above. I think these are the most pressing but luckily they are also the easiest to port. As far as I can tell they're mostly one-liners and none need to be adjusted for the API changes in the es6 branch.

Then there is of course the question of open PRs – I think bug reports specific to the es6 branch have not been flooding in, so I'm assuming that either people are not using it that much or that it is largely stable.

I have the feeling that there is still quite a bit of friction involved in maintaining this library – which kind of makes it difficult to do small bits of work on it because it is necessary to think of lots of things at the same time. Maybe it would be a good idea to automate more of the process:

  • Documentation deployment to the website (I know I keep saying this and don't come up with a solution myself 😋 )
  • Changelog generation (Maybe we could use something like commitizen to make this smoother)
  • Also I know this might be controversial but we could adopt the policy of not merging PRs with new features if there aren't any tests included

@mspae
Copy link
Contributor Author

mspae commented Aug 18, 2017

First PR for the first part is out – I checked approximately half of the commits that were different, here is the list

This list includes all commits that are different that are later than "Commits on Mar 12, 2017"

@mspae
Copy link
Contributor Author

mspae commented Aug 19, 2017

Second PR is out (#1193). I checked the other half. The only unmerged ones (the red circles) are plugin changes. I also didn't merge all the changelog changes so I can simply copy the current changelog in one go.

@mspae mspae removed the v2 label Sep 2, 2017
@thijstriemstra
Copy link
Contributor

Updated one of my plugins and wasn't too much work: https://github.com/collab-project/videojs-wavesurfer/compare/wavesurfer2

When can we expect a next release?

@katspaugh
Copy link
Owner

How about this weekend? Version 2.0.0 without the beta, right?

@thijstriemstra
Copy link
Contributor

A beta 2 would not hurt (or RC1).

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Sep 18, 2017

@mspae can you roll a new release (beta2) for the current master? I have some things I want to test.

@mspae
Copy link
Contributor Author

mspae commented Sep 22, 2017

@thijstriemstra I can only tag, I cannot publish to NPM. AFAIK only @katspaugh can do that.

@thijstriemstra
Copy link
Contributor

Ping @katspaugh

@katspaugh
Copy link
Owner

katspaugh commented Sep 22, 2017

Tagged & published!

@thijstriemstra thijstriemstra changed the title Releasing 2.0.0-beta01 Releasing 2.0.0 beta Sep 22, 2017
@thijstriemstra
Copy link
Contributor

Awesome thanks!

@thijstriemstra
Copy link
Contributor

Well it (still) works fine, so I'm +1 on a final release this week!

@thijstriemstra thijstriemstra changed the title Releasing 2.0.0 beta Releasing 2.0.0 Sep 29, 2017
@thijstriemstra
Copy link
Contributor

@mspae @katspaugh can we set a release date for 2.0.0? 1st of october? tomorrow? I'll help out where I can.

@mspae
Copy link
Contributor Author

mspae commented Sep 30, 2017

So I'm in two minds about this: On the one hand staying in beta makes it clear that there may be bugs we don't know about, on the other hand this is really also the case for a proper release. Stability is subjective and relative 😸

Also there is the matter of semver: releasing 2.0.0 means we can't break backward compatibility until we release 3.0.0 so we need to be really sure that this won't happen before we make the final step and go out of beta.

In light of this I would propose we do this:

  • Merge Visual regression testing #1210 and release as beta 3.
  • Wait until we are sure there is no major breakage. During this time …
  • Then IMHO we can release 2.0.0 because regions plugin refactor shouldn't have caused any breakage we don't know about because of all the tests which are in place (hopefully) by that time.

As a timeplan proposal: I think we should do the first step (see above) ASAP then aim to release 2.0.0 by the 10th of october.

I'm also working on a major overhaul of the website (more docs, more consistent design, bootstrap v4 etc) – it will also fix #1090 – I'm about 60% of the way and want to have this finished by the 2.0.0 release.

So that would be my proposal. This is of course open to discussion. I would be really happy for help with any of the above.

UPDATE: Updated the prettify point with the PR for that.

@thijstriemstra
Copy link
Contributor

#1210 changes the way ready event works and im -1 on these changes. Ill give feedback there. Secondly, this has been in beta since march and without a new release this project seems dead.

@thijstriemstra
Copy link
Contributor

Also, freezing API till v3.0 and all regtession testing seems way too strict. Its useful but it changes the dynamic of this project where before it was loosely maintained and released as often as possible. Having all this testing also requires reviewers and we don't have that manpower or userbase to help out from my experience with this project for last couple of years.

@mspae
Copy link
Contributor Author

mspae commented Sep 30, 2017

I totally agree with the "we need more releases" position.

I'm not proposing we stop adding features until 3.0.0, but to follow semantical versioning we can't break backward compatibility between releases in a major version. And for this purpose I think it is essential to have testing in place to make sure new features don't break existing functionality.

I don't quite see testing conflicting with loose maintenance. In my view it is absolutely necessary to be able to quickly add features and merge PRs, the alternative would be to either manually test any new code (which IMHO conflicts with the "loosely maintained" concept) or to simply hope nothing breaks (which means there can never be any reasonable amount of confidence in the stability of releases)

So yes, adding and maintaining tests is overhead which doesn't add features. I see that. But on the other hand it allows us to delegate a lot of the tedious part of the reviewing process (testing functional stability) to the CI system. This frees contributor resources which can be used for architectural discussion, documentation, support and new features. So I think testing actually saves a lot of time in the long run.

@mspae
Copy link
Contributor Author

mspae commented Sep 30, 2017

I understand your argument regarding the breaking changes in #1210 – It does delay the potential 2.0.0 release date because we need to make sure breakage does not occur. But for the reasons stated above I think it's worth it. In addition to the testing itself and the changes of the events the PR also contains a lot of fixes for bugs which were never reported because they only appear in certain param combinations and interactions. Having this kind of testing prevents these bugs from cropping up unnoticed in future.

What I can offer as a compromise:

  • I close the PR
  • we release v2
  • I open PRs for the rendering fixes (see above) and the visual regression testing without changing the way events are handled. (Some of the fixes rely on the consistent waveform-ready/ready event firing, but we can make it work somehow)
  • We release those as patch versions of 2 because they don't break anything

This way we can release immediately. What do you think?

@thijstriemstra
Copy link
Contributor

@mspae What's the status now? I think we should really release this asap (before end of year) because I feel it's damaging the project that there hasn't been a new release in such a long time. @katspaugh thoughts?

@mspae
Copy link
Contributor Author

mspae commented Dec 16, 2017

@thijstriemstra That's what I meant in #1092 (comment) – we can release now as far as I'm concerned. But I don't have publishing rights, so it's not up to me.

@thijstriemstra
Copy link
Contributor

@katspaugh seems to be missing in action 😨

@katspaugh
Copy link
Owner

katspaugh commented Dec 16, 2017

@thijstriemstra sorry, missed your previous message.
Publishing!

Edit: I've removed the beta postfix and published the package (see 4a93027).

Congratulations and thanks a lot!

@thijstriemstra
Copy link
Contributor

Thanks! 2.0.0 is now available on https://www.npmjs.com/package/wavesurfer.js, on to next release!

@thijstriemstra
Copy link
Contributor

@katspaugh unfortunately it seems something went wrong with the release, the dist folder is missing from the npm package:

$ ls node_modules/wavesurfer.js/
CHANGES.md  LICENSE  package.json  README.md  src

@thijstriemstra
Copy link
Contributor

People starting to notice: #1273

@thijstriemstra
Copy link
Contributor

I opened #1274 that will build the dist folder when you run npm publish, can you take a look @katspaugh and release a new 2.0.1?

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

No branches or pull requests

3 participants