[Question] frequency of _reparseObj seems like it could be excessive

[znewsham]

Not a bug, but a question - there are a bunch of places where setValueForPosition is called, including in a loop at the end of the forEachNode function. In the situation that a setValueForPosition necessitates calling _reParseObj - does this have to happen before the (immediately) next call to setValueForPosition executes? E.g., in forEachNode

    // Actually update/remove values as instructed
    Object.keys(updatedValues).forEach((position) => {
      this.setValueForPosition(position, updatedValues[position]);
    });

Could the _reParseObj call be made after all these setValueForPosition are complete?

[github-actions]

Thank you for submitting an issue!

If this is a bug report, please be sure to include, at minimum, example code that will reproduce the issue. Even better, you can link to a saved online code editor example, where anyone can immediately run the code and see the issue.

If you are requesting a feature, it is better to discuss it in the Discussions area first.

If you need to edit your issue description, click the [...] and choose Edit.

Be patient. This is a free and freely licensed package that we maintain in our spare time. You may get a response in a day, but it could also take a month. If you benefit from this package and would like to see more time devoted to it, you can help by sponsoring.

[aldeed]

@znewsham It has been a very long time since I wrote this code, but my gut says there are cases when it is necessary. For example, what if the updated value is itself an object or array? But might it be possible to isolate the exact circumstances that require a reparsing and be smarter about when it's done, yes.

The problem with this code is that it's very finicky and hard to understand the implications of any change. Overall though, my opinion is that if you change something and (1) all tests continue to pass and (2) all tests in the simpl-schema package continue to pass after updating that dependency, then it should be okay to merge. If it is not, someone will soon file a bug, we can add another test for that test case, and then work from there, potentially reverting the change.

So in other words, if someone wants to try this in a PR and prove that it doesn't break any tests, it should be okay to merge.

[znewsham]

Awesome - I'll have a crack at this - compared to the version in use by legacy meteor projects, this is already more efficient because it selectively runs the reparse - but looking at the code I was surprised it was still doing things this way as the new code is so much more performant than the old code.