addModifier parameter order?

ronyeh commented 2 years ago

Hi @rvilarl @0xfe

I'm just curious why the order of parameters in Note.addModifier() swapped places recently. I see mention of it in the recent circular dependencies PR, but I don't see how changing the order of the params fixes a circular dependency. Can you help me understand? Thanks!

The swap happened here: https://github.com/0xfe/vexflow/pull/1279/commits/37a9fdea9a191bb15b444e2b517bae3cfb680c9c#diff-d6198cff69fcf66b5a1c2b4d4f8e2101a5e9c64dc24bae1a904d49abc2ae4f0cR538-R541

Here is what the method looked like in previous releases: GITHUB:

UNPKG:

The current version is: https://github.com/0xfe/vexflow/blob/b404958b192f84d65043f5697ac3c22de2d0d881/src/note.ts#L535

So as far as I can tell, since version 1.2.0, the modifier was first and the index second.

I know the comments indicate otherwise, and when I was porting this file I thought it was curious which "legacy" versions of VexFlow had the parameters swapped. I never bothered to check.

So if it has always been addModifier(modifier, index), according to the old releases, then we should probably stick with it unless there's a great reason to swap now.

rvilarl commented 2 years ago

Hi Ron,

this is the reason: 1) in order to remove the dependencies with Dot and Accidental the following functions were removed from note:

  // Helper function to add a dot on a specific key
  addDot(index: number): this {
    const dot = new Dot();
    dot.setDotShiftY(this.glyph.dot_shiftY);
    this.dots++;
    return this.addModifier(dot, index);
  }

  // Convenience method to add dot to all keys in note
  addDotToAll(): this {
    for (let i = 0; i < this.keys.length; ++i) {
      this.addDot(i);
    }
    return this;
  }

  // Get all accidentals in the `ModifierContext`
  getAccidentals(): Accidental[] {
    return this.checkModifierContext().getMembers(Accidental.CATEGORY) as Accidental[];
  }

  // Get all dots in the `ModifierContext`
  getDots(): Dot[] {
    return this.checkModifierContext().getMembers(Dot.CATEGORY) as Dot[];
  }

2) as a consequence the following functions were also removed to be consistent and this functions added only an addtional call in the stack

  // Helper function to add an accidental to a key
  addAccidental(index: number, accidental: Modifier): this {
    return this.addModifier(accidental, index);
  }

  // Helper function to add an articulation to a key
  addArticulation(index: number, articulation: Modifier): this {
    return this.addModifier(articulation, index);
  }

  // Helper function to add an annotation to a key
  addAnnotation(index: number, annotation: Modifier): this {
    return this.addModifier(annotation, index);
  }

3) you can see that the order of the parameters of the functions in 2) differ from the order in addModifier one way or the other it was 100s of calls I unified to the order of 2) because the number of changes were less.

Hope this clarifies the reasoning behind. One way or the other it has an impact in the API.

ronyeh commented 2 years ago

OK thanks for explaining.

For the historical record, in version 3.0.9 there was an API design issue:

StaveNote extends StemmableNote StemmableNote extends Note

StaveNote defines addModifier as: addModifier(index, modifier)

https://unpkg.com/browse/vexflow@3.0.9/src/stavenote.js

Note defines addModifier with the parameters reversed: addModifier(modifier, index = 0) {

https://unpkg.com/browse/vexflow@3.0.9/src/note.js

It looks like it has been this way since 1.2.6: https://unpkg.com/browse/vexflow@1.2.6/src/note.js https://unpkg.com/browse/vexflow@1.2.6/src/stavenote.js

So if we are trying to unify all the addXXX() methods, there will be a breaking API change either way.

But since we are breaking APIs anyways.... Which one of the following seems better?

A) addXYZ(index, xyzInstance) B) addXYZ(xyzInstance, index)

I actually think option B makes more intuitive sense. When I type addXYZ(, I usually want to provide a xyz object immediately afterward. Option B also allows us to make the index optional (default: 0) so we can omit the second parameter if we are just adding one modifier.

It looks like stave.ts also has a addModifier(modifier: StaveModifier, position?: number).

So at the moment, note.ts and stave.ts each have a addModifier() method, but their arguments are reversed.

My vote would be to flip the argument order in note.ts, so that they are consistent with how they were in versions 1.2.6...3.0.9.

rvilarl commented 2 years ago

My vote would be to leave it as it is because there is higher use of addAccidental, addAnnotation and addArticulation than addModifier. I would also avoid to do the hundreds of changes again, at least manually :). Now that we are changing this API I would also rename addModifer in Stave to addStaveModifier.

sschmidTU commented 2 years ago

Option B also allows us to make the index optional (default: 0) so we can omit the second parameter if we are just adding one modifier.

This seems like an important argument. In a lot of cases you wouldn't need to add indices when adding modifiers, it would just be a hassle for the dev using the API, always incrementing the index, etc.

ronyeh commented 2 years ago

higher use of addAccidental, addAnnotation and addArticulation than addModifier

But you said these methods were removed?

If we go with the modifier instance first, index second, I'm happy to submit the PR for it.

... though I think I see what you mean now. If we consider the impact this breaking change will have on existing developers who are trying to upgrade to VexFlow 4.0 from 1.2.93, then they will have to do more work to fix their code (if they made heavy use of addAccidental, addAnnotation, addArticulation).

If we leave it as is, then developers who are upgrading only need to do a search and replace to rename calls to those three methods (addAccidental => addModifier, addAnnotation => addModifier, addArticulation => addModifier).

I'm still leaning towards having the modifier instance go first though. I think the Stave class has it right:

  addModifier(modifier: StaveModifier, position?: number): this {

ronyeh commented 2 years ago

I turned on the circular reference tool, and it looks like those three methods didn't contribute to the circular references. They took a Modifier object as a parameter, so it didn't add any links to chain of references.

  // Helper function to add an accidental to a key
  addAccidental(index: number, accidental: Modifier): this {
    return this.addModifier(index, accidental);
  }

  // Helper function to add an articulation to a key
  addArticulation(index: number, articulation: Modifier): this {
    return this.addModifier(index, articulation);
  }

  // Helper function to add an annotation to a key
  addAnnotation(index: number, annotation: Modifier): this {
    return this.addModifier(index, annotation);
  }

We could argue that those methods were not very useful.... :-) However, they do not cause circular references, even if we replaced Modifier with the imported types Accidental, Articulation, and Annotation.

Even though those look like references, those classes are just used as types, so they are erased when the TypeScript compiler processes it.

Before compilation:

  // Helper function to add an accidental to a key
  addAccidental(index: number, accidental: Accidental): this {
    return this.addModifier(index, accidental);
  }

  // Helper function to add an articulation to a key
  addArticulation(index: number, articulation: Articulation): this {
    return this.addModifier(index, articulation);
  }

  // Helper function to add an annotation to a key
  addAnnotation(index: number, annotation: Annotation): this {
    return this.addModifier(index, annotation);
  }

After compilation (vexflow-debug.js):

// Helper function to add an accidental to a key
addAccidental(index, accidental) {
    return this.addModifier(index, accidental);
}
// Helper function to add an articulation to a key
addArticulation(index, articulation) {
    return this.addModifier(index, articulation);
}
// Helper function to add an annotation to a key
addAnnotation(index, annotation) {
    return this.addModifier(index, annotation);
}

Here was the block that was removed:

https://github.com/0xfe/vexflow/pull/1279/files#diff-d6198cff69fcf66b5a1c2b4d4f8e2101a5e9c64dc24bae1a904d49abc2ae4f0cL566-L604

The following definitely DOES contribute to circular references:

  getAccidentals(): Accidental[] {
    return this.checkModifierContext().getMembers(Accidental.CATEGORY) as Accidental[];
  }

This makes a hard reference to a static member inside the Accidental class (Accidental.CATEGORY), which will not be erased by the TypeScript compiler & webpack.

Compiled code:

getAccidentals() {
    return this.checkModifierContext().getMembers(_index__WEBPACK_IMPORTED_MODULE_0__.Accidental.CATEGORY);
}

Notice the type information Accidental[] and as Accidental[] was erased.

Summary:

class names used just as type information don't contribute to circular dependencies because they are erased during compilation.
accessing a class's static member (e.g., Accidental.CATEGORY) adds hard references, and could contribute to a broken build at runtime.

Circular references are not always immediately problematic. If they break the runtime tests, like they did in December, then we should definitely diagnose and resolve them.

I think the typeguard methods and .CATEGORY approach definitely could be improved. The isCategory() method uses instanceof, which is one of the hard references that causes circular references.

Anyways, thanks for working on this @rvilarl ... I definitely learned something while trying to understand circular references better.

rvilarl commented 2 years ago

higher use of addAccidental, addAnnotation and addArticulation than addModifier

But you said these methods were removed?

If we go with the modifier instance first, index second, I'm happy to submit the PR for it.

... though I think I see what you mean now. If we consider the impact this breaking change will have on existing developers who are trying to upgrade to VexFlow 4.0 from 1.2.93, then they will have to do more work to fix their code (if they made heavy use of addAccidental, addAnnotation, addArticulation).

If we leave it as is, then developers who are upgrading only need to do a search and replace to rename calls to those three methods (addAccidental => addModifier, addAnnotation => addModifier, addArticulation => addModifier).

This is what I meant, @sschmidTU In OSM I thought that the prefered approach was the one in addAccidental due to the comments stating "Vexflow made a mess with the addModifier signature that changes through each class so we just cast to any :(" This casting also introduced bugs because the parameters were in some instances in the order expected by addAccidental.

I'm still leaning towards having the modifier instance go first though. I think the Stave class has it right:
  addModifier(modifier: StaveModifier, position?: number): this {
The problem with this addModifier in Stave is the name. StaveModifier does not extend Modifier it extends Element as a consequeence I think that addStaveModifier(staveModifier: StaveModifier, position?: number): this { would be clearer.

I think it is a matter of taste, having index at the end allows to make it optional and having it at the beginning is more in line with the lists add functions or splice in arrays. In any case I suggest to look for an automatic refactoring tool if we change it (there are 762 references in VexFlow!) @0xfe which order do you prefer?

ronyeh commented 2 years ago

Either parameter order is fine with me, but I would prefer if the ordering is more consistent.

We can rename Stave's method to addStaveModifier(), but the parameter order there is still 1) object instance, then 2) index/position which is optional.

In Note, it's the reverse. Whichever one we choose in the end, I'd prefer if both method signatures took the same approach.

If we decide to flip Note.addModifier(index: number, modifier: Modifier) => Note.addModifier(modifier: Modifier, index: number=0), I can volunteer to fix the 700+ instances. Maybe there is some magical regex that will help. :-) :-]

0xfe commented 2 years ago

This is partly my fault for starting this mess (sorry!), and I agree that it's super important to keep the the ordering consistent.

I think addModifier(modifier, ...) is the ideal approach from an API design perspective. Typically when there's a doSomethingWithNOUN method, the first parameter should always be a required NOUN. It allows for optional parameters after.

So unless there are any major objections, I propose that's what we do.

ronyeh commented 2 years ago

OK, I can tackle this one after my gruntfile PR is done.

I will have to research some magic regex :-).

rvilarl commented 2 years ago

@ronyeh this can be closed now

0xfe / vexflow

addModifier parameter order? #1294