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

Breaking changes v11 #2476

Closed
11 tasks done
josdejong opened this issue Mar 10, 2022 · 30 comments
Closed
11 tasks done

Breaking changes v11 #2476

josdejong opened this issue Mar 10, 2022 · 30 comments
Milestone

Comments

@josdejong
Copy link
Owner

josdejong commented Mar 10, 2022

For the first next breaking version v11 release, the following changes are scheduled:

If there are more breaking changes that should be taken care of in this release, please drop a message here

@josdejong josdejong added this to the v11 milestone Mar 10, 2022
@gwhitney
Copy link
Collaborator

gwhitney commented Mar 14, 2022

Remove automatic number to string conversion (see #2437 discussion). It's PR #2482

@josdejong
Copy link
Owner Author

Thanks @gwhitney 👍

Instead of keeping the PR's open until all of them are done, I will create a v11 branch and merge the PR's there as soon as they are ready.

@gwhitney
Copy link
Collaborator

Also am working on a PR for #2485 to go into this release. It will require publication of a new version of typed-function as well, according to the plan.

@gwhitney
Copy link
Collaborator

possibly #2422 as well?

@josdejong
Copy link
Owner Author

Yeah good point. That is one of these "is it a bug fix or a breaking change" cases 😉

@gwhitney
Copy link
Collaborator

I just proposed #2490 which if you concur should be added to the list for this release.

@gwhitney
Copy link
Collaborator

Ok, now that typed-function has v3 published we should be able to finish this off. I have v11_on_v3a which will run on top of it. I will rebase it on develop and add a commit that officially updates to typed-function v3 and confirm that here.

But then it has a lot of changes as compared to the prior v11 branch. How do you want to go about reviewing? Or do you want some other strategy for rebuilding all of the v11 stuff on top of current develop and typed-function v3?

I will wait to implement the last task, not chaining a rest parameter across the internal value and new arguments, until we have some plan for how to get this across the finish line. Looking forward to your thoughts.

@josdejong
Copy link
Owner Author

Yes, v11 can move on now 👍

Hm, I'm afraid it will be painful one way or the other. I would prefer the approach that would be least amount of work, but I'm not sure if that is solving merge conflicts from v11 to v11_on_v3a, or start clean and recreate v11_on_v3a from scratch, copy/pasting as much as possible.

I've brought v11 up to date with the latest develop now.

@gwhitney
Copy link
Collaborator

Well, I have now brought v11_on_v3a up to date with the latest develop as well, and it builds and passes all tests. It is precisely v11 followed by a series of commits to make it work on top of typesecript v3.0.0 (as v3a was published). There were no merge conflicts in rebasing on develop (there had been merge conflicts on previous rebases, but not this time).

So as far as I am concerned, v11_on_v3a should be the new v11 and it is ready to go except for the last unchecked task on the list here. Do you feel there is any way you can review v11_on_v3a? Or what would allow you to do that? Again, I will wait to do the last task for the version 11 release until we've settled on what the foundation for the release will be.

I do not think there is any point in making a version of v10 on top of typed-function v3.0.0; the refactoring benefitted a lot from the removal of a bunch of possibly ambiguous matrix functions in the version 11 branch.

Then main refactor from the v11 branch to the v11_on_v3a branch is collapsing lots and lots of matrix/array operation boilerplate into the new "matrixAlgorithmSuite" utility where you just specify which of the algorithms to use for Sparse-Dense, Sparse-Sparse, Sparse-scalar, etc. (I also added mnemonics after the number for each of the matrix algorithms to keep myself sane as
I was doing that refactor.) That greatly reduced the total number of lines of code in mathjs, because there were a lot of very repetitive matrix implementations of operations, and it greatly reduced the number of instances of 'this', simplifying the converson to typed-function v3.

Otherwise each step is fairly minor and I tried to log them all reasonably clearly. I think you will see in the git log for v11_on_v3a that it is quite clean: it is just current develop, followed by the v11 commits through PR #2490, followed by the commits to adapt to typed-function v3. So I am hoping that it will be plausible for you just to review v11_on_v3a and for us to proceed with that. I expect you will have some requested changes, but I am hoping not too many to deal with, and other than any of those, I think v11_on_v3a is ready to get the last PR for the desired fix to chaining and then good to go. Thanks for taking a look and letting me know what you think.

@gwhitney
Copy link
Collaborator

All that said, I realize that v11_on_v3a is a dozen commits on top of v11, so let me know anything I can do to facilitate your review of all that happened in going from v11 to v11_on_v3a.

@gwhitney
Copy link
Collaborator

Ah, I still had an Object.hasOwn to protect against namespace pollution, but that's not supported in Node 14; also, removed 'resolve' from the list of known functions that fail the doc test, now that it handles strings. Hopefully, all should be working now.

@gwhitney
Copy link
Collaborator

Ugh, IE 11 is failing with an 'expected ;' error in the browser tests for v11_on_v3a. It's pretty hard for me to decipher the output; can you have a look? I think maybe the problem is actually in typed-function, not mathjs? 32 days to go until IE 11 EOL... ;-)

@josdejong
Copy link
Owner Author

Thanks. Great to here there where no nasty merge conflicts in updating v11_on_v3a.

It is what it is, it will simply be a big PR, I will go through it coming Monday and have a look at the latest individual commits related to the typed-function@3 upgrade.

It makes sense to me to go for branch v11_on_v3a , merge that in develop, and will delete v11. And indeed we should not publish a v10 with [email protected] (breaking change in a dependency should be published as a breaking version, v11 in our case).

I'll have a look into the IE11 issue. Yeah can't wait until EOL 😂 . I saw the latest version of mocha@10 already dropped support for IE11 very recently 😅 .

@josdejong
Copy link
Owner Author

Ok... I opened typed-function/test/browser.html on IE11 (using BrowserStack), and indeed it just doesn't run on IE11. In the code there are a couple of for (... of ...) { ... } which is not supported. When releasing [email protected] I totally forgot to run on IE11 😳.

There are 13 occurrences of for (... of ...) { ... }. Honestly, it would be a pity to rewrite them to old JS code for the coming 4 weeks, and then write everything back to again. How about we add a line to the changelog of v3.0.0 that support for IE11 was dropped (apparently it was dropped in that version, we only forgot to communicate it 😉). I do not think it is a problem if we publish mathjs@11 droping IE11 support a few weeks before the official EOL. What do you think?

@gwhitney
Copy link
Collaborator

I mean, no objection from me -- I am sure I am the main culprit in not being able to keep track of which dialect of JavaScript was OK 7 years ago... Sorry about that. But so I have no problem dropping IE11 support, entirely up to you.

@gwhitney
Copy link
Collaborator

And let me know if I should push a commit to v11_on_v3a removing bs_ie_11 from browserstack_karma.js.

@josdejong
Copy link
Owner Author

👍

Yes lets drop support for IE11 in mathjs@11. I've added this to the list on top of this issue.

@gwhitney
Copy link
Collaborator

Done. Please let me know if I should go ahead with the chaining restriction commit, or if you want to look this much over first.

@josdejong
Copy link
Owner Author

Awesome. Can you implement the chaining restriction in a separate feature branch that we can merge into v11_on_v3a?

@gwhitney
Copy link
Collaborator

Yup, will do.

@gwhitney
Copy link
Collaborator

On the most recently added possibility, #2617, I registered my vote against it in the relevant issue.

@josdejong
Copy link
Owner Author

I've described all (breaking) changes in the history: https://github.com/josdejong/mathjs/blob/develop/HISTORY.md#not-yet-published-version-1100. @gwhitney can you have a look to see if the list is complete?

I think all is ready for the release of v11, except for waiting for feedback on #2617 and finishing that up. I'm planning to publish v11 coming Friday.

@gwhitney
Copy link
Collaborator

Comments on the HISTORY.md list you link in the previous comment:
A) In the 3rd bullet point I would give an example, like "Example: formerly -1 / 2 b was interpreted as -1 / (2b) and now it is interpreted as (-1/2)b."
B) I'd suggest clarifying in the 8th bullet point as to what is the resulting breaking behavior of simplifyCore. The most significant difference is that simplifyCore will no longer (partially) merge constants; that behavior has been moved to simplifyConstant. So math.simplifyConstant(math.simplifyCore(expr)) is close to the old behavior of simplifyCore, although still not exact because of the interaction of the two operations. math.simplify(expr, [math.simplifyCore, math.simplifyConstant]) will do everything the old simplifyCore did and be more thorough about reducing constants.
C) In the ninth bullet point, I might give an example of what "splitting parameters" means: "Example: math.chain(3).max(4, 2).done() will now throw an error rather than return 4, because the rest parameter of math.max(...number) has been split between the contents of the chain and the arguments to the max call."
D) In the tenth bullet point, about Function/function, I might add the explanatory note "for consistency with JavaScript typeof"

Otherwise all looks good to me.

@josdejong
Copy link
Owner Author

Thanks, good point to clarify those points with examples and more description. I've update the history accordingly (see 340a6f6).

@gwhitney
Copy link
Collaborator

Seems good to me. Looking forward to Friday's publishing of this (and fingers crossed it won't suddenly turn up a bunch of bugs...). Also looking forward to your thoughts on the Pocomath prototype I posted yesterday, in terms of the next direction of mathjs.

@josdejong
Copy link
Owner Author

Yes I saw your Pocomath post 👍 . I love the name already but I still have to take the time to read your comments and understand the approach.

@gwhitney
Copy link
Collaborator

In #2628, @costerwi points out that it should be possible to construct a Unit from a numeric value and a valueless Unit, whereas currently it only works with strings. I agree, but in fact on reflection, I don't actually understand why that constructor should take a string there at all; what the constructor needs is a value and the units for that value. For example, new Unit(5, 'blarf') just doesn't make sense, which to me is a semantic indication that an argument of type string isn't right there, as opposed to, say, new Error('blarf'). So in the presence of Unit.parse, it seems to me that the two-argument class constructor should only take a value and a valueless unit, and if you want the convenience of supplying a string, use the mathjs function math.unit(5, 'km') since that sort of convenience is after all one of the things that the mathjs functions are for. That doesn't mean every convenience needs to be supported in the underlying class, especially when it clutters the code to do so.

Anyhow, if it happens that (a) you agree that the two-argument Unit constructor should only take a valueless Unit (or undefined) in the second position, not a string, and (b) you would consider that a breaking change (not sure whether the Unit constructor is considered part of the mathjs API or not, especially given the plans I've heard to move Unit out of mathjs), then do you want to squeeze that into V11? If so, I'd be happy to take @costerwi's PR and rebase it right away (or perhaps @costerwi would like to do so) so that it can be incorporated in the V11 release. Let me know.

@josdejong
Copy link
Owner Author

Interesting to think that through. Let's discuss in #2628 and wait with releasing v11 until that is settled.

@josdejong
Copy link
Owner Author

We'll not introduce breaking changes in #2628. Going to publish v11 now.

@josdejong
Copy link
Owner Author

Ok v11 is published now.

@gwhitney thanks for the huge amount of time and effort you've put in this release!

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

2 participants