Thanks Carl! This definitely makes sense. I'll review your PR within a few days.

Thanks for the comments! The approach of removing string as an option for the second argument in the constructor makes sense to me. I'll update the PR in that direction.

Ok that approach makes sense to the two of us but @josdejong has to approve and then it has to be determined whether that's a breaking change that we're going to try to stuff into v11, currently due to be published tomorrow. So it would be great for you to prepare that as long as you're comfortable that it might have to be rolled back or redone if Jos prefers a different approach. If you want to be sure not to make effort that won't be used, you should wait for Jos's thoughts. I am a maintainer but Jos is the founder and architect.

@gwhitney yes it makes sense to me to see if we can simplify the constructor, it's a bit cluttered indeed and there are more ways to do the same thing than probably needed.

So, before this PR, you could create a unit in the following ways:

new Unit(null, 'cm') // create a valueless unit "cm"
Unit.parse('cm')     // create a valueless unit "cm"
new Unit(5, 'cm')    // create a unit "5 cm"
Unit.parse('5 cm')   // create a unit "5 cm"

I understand the latest proposal is to replace the second argument to only accept a Unit:

new Unit(null, Unit.parse('cm')) // create a valueless unit "cm"
Unit.parse('cm')                 // create a valueless unit "cm"
new Unit(5, Unit.parse('cm'))    // create a unit "5 cm"
Unit.parse('5 cm')               // create a unit "5 cm"

Needing a Unit to create a Unit feels a bit like a weird circular case. I think ideally, the constructor should contain hardly any logic. Looking at how Unit.parse is currently used inside the constructor:

      const u = Unit.parse(name)
      this.units = u.units
      this.dimensions = u.dimensions

Shows though what we actually need under the hood: value, units and dimensions.

So, how about changing the constructor to:

function Unit (value, units, dimensions) {
  // ...
}

What do you think?

EDIT: that makes the constructor less usable in general, but more flexible. For practical use cases you'll need a utility function like Unit.parse or math.unit, and we can easily add more typical use cases there, like the new idea to be able to create a Unit from a value and a Unit.

I definitely like the idea of making the Unit constructor the most semantically straightforward thing -- the "natural" way to create a unit. On the other hand, the vector of units and dimensions feels more like the internal representation than the natural specification of a unit. For example, unless I am mistaken, the vector of units and the dimensions have redundant information. I believe the dimensions vector can be determined by the vector of unit objects (add up the powers of each base_unit represented in the vector of units). So it would seem that for constructing, at least you would/should only need to specify the vector of units. My second concern is that the vector of units is in a pretty opaque form: it is an array of objects with keys 'unit', 'prefix', and 'power'; the values for the 'unit' key is an item from the Unit.UNITS object, the 'prefix' is an item from the 'prefixes' property of the 'unit', and 'power' is an integer. (I don't think a square root of an area unit like an acre is allowed.. ;-)

In short, even just the vector of units might be a bit too "internals"-ish and cumbersome as a constructor. To actually use it directly, for 3 meters^2 per millisecond, you'd need to write something like:

new Unit(3, [{unit: Unit.UNITS.meter, prefix: unit.UNITS.meter.prefixes[''], power: 2},
             {unit: Unit.UNITS.second, prefix: unit.UNITS.second.prefixes.milli, power: -1}])

which is a bit of a mouthful. Maybe that's OK though, or would it maybe be better to have one inclusive library of prefixes and allow something like:

new Unit(2, [[Unit.UNITS.meter, 2], [Unit.PREFIX.milli, Unit.UNITS.second, -1]])

(where the constructor uses a conventional order of: optional prefix, then unit, then power -- to avoid all those keys, and it can do the work to error-check that the prefixes are actually compatible with the units (i.e. no 'gigaft') and assemble it into its convenient internal rep, reconstructing the dimensions property?)

In a slightly related point -- I hesitate to even bring it up at this juncture, so feel free to just completely ignore this question if it doesn't seem reasonable to contemplate at the moment, or seems ridiculous -- would the structure/code/usage here actually be cleaner/simpler if ValuelessUnit were its own class (probably one could find a better name) and then a Unit were simply a pair of a numeric entity and a ValuelessUnit? [I'd be tempted to call them a Unit (which would be the valueless one) and a Quantity (= a numeric value with a Unit), personally.] The points here are that one can delegate the details of specifying a ValuelessUnit to that class (whatever it's called), and then the UnitWithValue constructor takes the quite natural (value, valuelessunit) pair without that smacking of a weird recursion, and it would remove the slightly odd ambiguity as to whether a given Unit instance represents an actual quantity or just a way of measuring such quantities, i.e. in grams. I feel like that current ambiguity leads to a lot of branching in the code that might be unnecessary if the two classes were separated.

Anyhow, just some thoughts given that the can of worms is at least slightly open; I definitely trust that you will steer a reasonable course here, especially in light of the thought to split off Unit from mathjs, which you probably know more about the state of than we do.

It sounds like some good ideas for discussion. I especially like the distinction between Unit and Quantity!

Thanks for your inputs Glen. Yes I have the same feeling: it will not help opening up too much "internals". In the end I hope we can integrate https://github.com/ericman314/UnitMath here, which as a very neat and simple API already.

An API like new Unit(2, [[Unit.UNITS.meter, 2], [Unit.PREFIX.milli, Unit.UNITS.second, -1]]) sounds indeed neat and powerful, that could be something.

Part of the problem is indeed that we now try to represent two different things in the same class: a unit with value and a valueless unit. Great idea to name them Unit and Quantity, that is a very clear naming. @ericman314 how did you solve and name these things in UnitMath again? Is it still an issue there?

I've been thinking about whether it's worth to make a breaking change in the existing constructor API for this. At this point, I have the feeling we shouldn't over-engineer this. The idea of the PR was to add one more handy way to construct a Unit. I like pure, clean API's, and the current Unit constructor could use some improvements. I think refactoring into a separate Unit and Quantity, or into a neat, proper array with units like Glen idea will be very neat, but this will result in a big refactor, too big for now I think. Ideally, these ideas would be addressed by UnitMath in one go. It think it is best at this point to keep the existing code working and extend it with this new option. There is a lot of value in backward compatibility too. And honestly, I do like the practical constructor where you can just pass a string too.

Can you guys agree to leave the existing behavior as is for the time being?

I've implemented the suggestions from @gwhitney which don't break anything.

The UnitMath project has a setValue() method which "Returns a copy of this unit but with its value replaced with the given value." This could be an alternative to enhancing the constructor but I would have named it withValue() which sounds more like it will leave the original object unchanged and return a copy.

Sorry for being late to the discussion. I would love to get UnitMath ready. It's based off of Unit.js, the API has been simplified and well documented, and we migrated it to TypeScript. It's hung up on just one or two things relating to making it work with custom units and formats (as in, extending it to work with libraries like Decimal.js, etc). I was very close to getting v1 released--and then life got busy, as they say. I would love to get it integrated with Math.js before the two libraries diverge too much. I think with some help we could get UnitMath v1 released pretty soon.

If you would like to help, the place to start is the v1 branch of UnitMath. That has all of the latest changes I and a few others have been working on. If you have time, please check it out. We can definitely implement the Unit from Unit feature there as well.

Well, something in the new constructor has borked Unit.multiply. @josdejong, given that the decision is to leave the Unit(value, string) constructor for now, I suggest not waiting to release v11. That way @costerwi can fix this when feasible and merging this won't be a breaking change, so can be in v11.1. I left my review comments alone even though they are moot, as a record of thoughts for the Unit api. But if you want to mark them all resolved instead, go ahead, or I can if you want.

Thanks for your update @ericman314!

given that the decision is to leave the Unit(value, string) constructor for now, I suggest not waiting to release v11

Yes, I'll publish v11 now, and we can finish and merge this PR afterwards.

See the comment I just added; you'll need to restore the removed unit test before this can move forward. Thanks for your patience with this sort of thing, it's much appreciated.

I don't see anything other than the last comment I put in the code; am happy to merge as soon as we reach consensus on that and resolve it.

Just wanted to apologize, especially to @costerwi, about urging to remove the test when in fact the conclusion is it should be there. I will be happy to restore the test and merge the PR. Thanks for everyone's efforts!

No need to apologize. This kind of collaboration is how the software becomes the best it can be.

No problem @gwhitney, I appreciate your careful review and discussion!

Thank you all for your contributions to this very useful project.

Thanks guys 👍

Published now in v11.1.0, sorry for the delays, there was a holiday in between.