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

Improve stability of enum proxies #200

Closed
knuton opened this issue Jul 10, 2015 · 5 comments
Closed

Improve stability of enum proxies #200

knuton opened this issue Jul 10, 2015 · 5 comments
Assignees

Comments

@knuton
Copy link
Contributor

knuton commented Jul 10, 2015

Problem

When generating headers for a module, we currently regenerate verbatim each enum defined in a module dependency, see e.g. here and here.

When operating within one language we would not need to do this, but could directly import the enum from each foreign module. However, as we cross language boundaries, we need to make the enums comprehensible for each language.

The current solution of just regenerating the whole enum in the module language is problematic over time in the light of independent compilation. For example we start with

// A.module
enum Stooges {
  Moe
  Larry
  Curly
}
Stooges getStooge()
// AImpl.ts
function getStooge(): Stooges {
  return Stooges.Larry;
}

and refer to values of this enum in another module by name,

// BImpl.ts
switch (A.getStooge()) {
  case A.Stooges.Larry: // do something assuming Larry
}

Now the order of the cast changes slightly

// A.module
enum Stooges {
  Moe
  Shemp
  Larry
  Curly
}

while everything else stays the same.

Because A and B are compiled independently, as long as B is not recompiled after the changes to A, it will mistake the value it receives from A.getStooge() as not being Larry because its locally generated enum definition is out of sync (1 != 2). Clearly, this is conceptually a breaking change in the API and A's major version should be bumped.

Proposed Solution

There is however a way of reducing the impact of missed version bumps. While we clone enums from dependencies, we don't do this for functions exposed on dependencies: We merely create proxies that provide a typed interface. The same can be done for enums.

Instead of generating Stooges from above as

enum Stooges {
  Moe = 0,
  Larry = 1,
  Curly = 2
}

in the TypeScript headers for B, we use an indirection to the canonical values in B

enum Stooges {
  Moe = A.module.Stooges.Moe, // untyped, JS-level reference to underlying A implementation
  Larry = A.module.Stooges.Larry,
  Curly = A.module.Stooges.Curly
}

and thereby keep the value of each enum name stable across modules.

The enum proxy in B will still be out of sync after Shemp is added to A, but potential for breakage is reduced.

Second Thoughts

This may lead to developers being less motivated to bump major versions, which is still necessary: What if A.getStooge() changes to return Stooges.Shemp? B is unprepared and likely to break. Is it maybe still preferable to force major version bumps on changes to enums?

@knuton
Copy link
Contributor Author

knuton commented Jul 10, 2015

Prior art:

  • C# reference considers changes to enums a breaking change, see section "Robust Programming"
  • API Design wiki's take: It's not a breaking change, if everyone sticks to the contract of always thinking of enums as open and always provides a sensible default branch

@encse
Copy link
Contributor

encse commented Jul 10, 2015

Bumping always solves the problem, but since bumping is a pain in the ass people avoid it whenever possible. Most of the time they just fix everything when changing enum values. But it's easy to forget an empty commit to those modules that don't require source change, just server side recompile. This is often forgotten, and will be forgotten whatever we do.

The first solution fixes problems like this, so I vote for that. We should also suggest bumping when the change occurs in cross team module boundaries.

@lptr
Copy link
Contributor

lptr commented Jul 10, 2015

There are more benefits to shared enum instances, like using objects as enum values. That was the original implementation in Spaghetti 1.0, too. However, there's benefits in how enums (and constants) are handled now: simplicity. The only JS object your module gets from a Spaghetti dependency during runtime is the dependent module object, and only that needs to be exposed to your module's code—all the rest is in the generated code.

Also, designing for making breaking changes without administering to those breaking changes seems like a bad idea. When it comes to making changes to enum values, there are two options:

  • it's a breaking change: you need to rename or remove one of the enum values—this proposal is not going to fix that
  • it's a change that can be done in either a breaking or a non-breaking way, i.e. adding new values—in this case going with the non-breaking option sounds like the way to go. With the new ability to explicitly assign numeric values to enums it should not be hard either.

@knuton
Copy link
Contributor Author

knuton commented Jul 10, 2015

Also, designing for making breaking changes without administering to those breaking changes seems like a bad idea.

I agree.

Still, let's dissect this: There are three interesting cases removing, appending and reordering. (Adding is just appending + reordering. Renaming is just removing + adding.)

  • Removing can not be done without breakage with independent compilation and the need for proxies
  • Reordering can be done without breakage, just adapt the assigned values accordingly using explicit assignment
  • Appending can be done with moderate amounts of breakage: The pre-existing entries can be held stable. But the unexpected new cases can cause problems still.

For what it's worth, introducing the indirection would make it braindead to reorder without causing breakage. I don't have to think about it as a programmer, and there is nothing I can mess up by miscalculating assigned values.

That is a win. I am willing to give up some simplicity on the implementor side for this.

@lptr What were the complexities that made you move away from shared values originally? Would we have the same level of complexity if we would still restrict values to integers?

@knuton
Copy link
Contributor Author

knuton commented Jul 22, 2015

OK, I eventually bumped into complexity: The difference between enums from direct and transitive dependencies.

I pushed a preliminary solution with an explanation here: 92bdbed

@lptr Interested to hear your take on this

@knuton knuton closed this as completed in afa6bbc Jul 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants