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

Fix modification of options.time #56

Merged
merged 1 commit into from
Jan 27, 2019
Merged

Conversation

normanfeltz
Copy link
Contributor

The original options.time object was modified

The original options.time object was modified and create a bug
@djipco djipco self-assigned this Jan 26, 2019
@djipco
Copy link
Owner

djipco commented Jan 26, 2019

Thanks for the PR. Would you care to elaborate on the issue?

@normanfeltz
Copy link
Contributor Author

normanfeltz commented Jan 26, 2019

Hello,

A time: undefined appear in my object, the problem is only with note_on

This is my partial code :

function getInputsData() {
    return new Promise((resolve, reject) => {
        const type = $("#midi_type option:selected").val();

        if (type != "") {
            const args = {};
            const options = {};
            args["type"] = type;

            const params = types[type]["parameters"];

            [...]

            console.log(args);
            console.log(options);

            const result = {...args, options};
            console.log(result);
            
            resolve(result);
        }
        else {
            reject("Veuillez sélectionner un type valide !");
        }
    });
}

The output of the console :
Console

For debug i use this in strict mode :

Object.freeze(options);

With this, i have this error :

Console

This error got to :
Console

Cordialy

Norman FELTZ

@djipco djipco merged commit 0ffd968 into djipco:master Jan 27, 2019
@djipco
Copy link
Owner

djipco commented Jan 27, 2019

Ok, I totally understand now. The library shouldn't mess with the options object. The only reason this was done is because options.time is used twice in the playNote method. While I did merge your PR, I will push another change in a few minutes to make sure we account for both usages. Thanks.

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

Successfully merging this pull request may close these issues.

2 participants