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

feat(jsii-diff): add tool to check API compatibility #415

Merged
merged 13 commits into from
Apr 3, 2019

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Mar 31, 2019

Add parsing semantics for various common capabilities to doc comments
(summary, remarks, stability and others).

Add a tool uses stability annotations in the doc comments to check
whether two JSII assemblies are API-compatible.

Fixes #358


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Add parsing semantics for various common capabilities to doc comments
(summary, remarks, stability and others).

Add a tool uses stability annotations in the doc comments to check
whether two JSII assemblies are API-compatible.
@rix0rrr rix0rrr requested review from costleya, dstufft and a team as code owners March 31, 2019 18:46
build-js.sh Outdated Show resolved Hide resolved
packages/jsii-calc/test/assembly.jsii Outdated Show resolved Hide resolved
packages/jsii-diff/README.md Outdated Show resolved Hide resolved
packages/jsii-diff/README.md Outdated Show resolved Hide resolved
packages/jsii-diff/README.md Show resolved Hide resolved
packages/jsii-diff/lib/type-analysis.ts Show resolved Hide resolved
packages/jsii-diff/lib/type-analysis.ts Show resolved Hide resolved
packages/jsii-diff/lib/type-analysis.ts Show resolved Hide resolved
packages/jsii-diff/lib/util.ts Outdated Show resolved Hide resolved
packages/jsii-diff/lib/util.ts Show resolved Hide resolved
Rico Huijbers added 3 commits April 1, 2019 16:01
- Rename `@see` field back to 'see'.
- Treat unannotated APIs as `@stable` by default.
- Add feature to leave out NPM package name by default for very
  concise comparison against latest release: `jsii-diff npm:`.
- Split out struct vs ref interface comparisons to simplify.
- Split out tests for same reason.
- Take inherited constructors/methods into account.
- Some more explanations in readme.
- Fixed an oopsie, can't add required arguments to methods.
Merge remote-tracking branch 'origin/master' into huijbers/jsii-diff
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Apr 2, 2019

On the discussion of @subclassable:

I will lift it to a first-class annotation in the JSII spec. The idea being that JSII client generators will have the ability to look at this annotation and add sealed/final to classes, and potentially a @DoNotImplement annotation to interfaces which could be processed by an annotation processor.

On whether the annotation burden should be on @sealed or @subclassable:

  • Designing something for subclassing takes design effort. It makes sense therefore to consider classes/interfaces closed for subclassing by default, and only explicitly opening them up for subclassing by annotating them.
  • Consider what happens if you get it wrong? Opening up for subclassing == adhering to a stronger contract == a one-way door (you cannot opt out of subclassability without breaking compatibility). If the default is sealed and you figure out you got it wrong, you can make a class @subclassable without breaking backwards compat. If the default is open and you figure out you got it wrong, adding the @sealed annotation would require bumping the MV.

For both of these reasons, I think the default should be sealed and @subclassable should be the opt-in annotation.

packages/jsii-calc-lib/test/assembly.jsii Outdated Show resolved Hide resolved
packages/jsii-diff/bin/jsii-diff.ts Show resolved Hide resolved
packages/jsii-diff/bin/jsii-diff.ts Show resolved Hide resolved
packages/jsii-diff/bin/jsii-diff.ts Outdated Show resolved Hide resolved
packages/jsii-diff/bin/jsii-diff.ts Show resolved Hide resolved
packages/jsii-diff/package.json Outdated Show resolved Hide resolved
packages/jsii-diff/package.json Show resolved Hide resolved
packages/jsii-pacmak/lib/targets/java.ts Outdated Show resolved Hide resolved
/// <summary>getXxx() is not allowed (see negatives), but getXxx(a, ...) is okay.</summary>
/// <remarks>
/// remarks: ..) is okay.
/// summary: getXxx() is not allowed (see negatives), but getXxx(a, .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ftaghn?

@@ -81,7 +77,7 @@ async function loadPackageNameFromAssembly(): Promise<string> {
if (!await fs.pathExists(JSII_ASSEMBLY_FILE)) {
throw new Error(`No NPM package name given and no ${JSII_ASSEMBLY_FILE} file in the current directory. Please specify a package name.`);
}
const module = await fs.readJSON(JSII_ASSEMBLY_FILE, { encoding: 'utf-8' }) as spec.Assembly;
const module = spec.validateAssembly(await fs.readJSON(JSII_ASSEMBLY_FILE, { encoding: 'utf-8' }));
if (!module.name) { throw new Error(`Could not find package in ${JSII_ASSEMBLY_FILE}`); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't reckon this is possible anymore, not that you've run through validateAssembly :)

Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏻 Dope. Would be nice to have follow-up issues created for the couple of low-impact items you're leaving out (colored output, this kind of things).

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Apr 3, 2019

Done. Don't like what the C# generator is doing to the doc comments on final glance, so I'm going to have to dive in there to finish this off.

packages/jsii-diff/bin/jsii-diff.ts Show resolved Hide resolved
packages/jsii-diff/bin/jsii-diff.ts Outdated Show resolved Hide resolved
packages/jsii-diff/generate.sh Show resolved Hide resolved
}
}

if (original.docs.isSubclassable && !updated.docs.isSubclassable) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too :-(

}

export function compareStruct(original: reflect.InterfaceType, updated: reflect.InterfaceType, context: ComparisonContext) {
// We don't compare structs here; they will be evaluated for compatibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks weird. wouldn't it make sense to just have an argument that says if this is an input/output struct? [and again I am going back to the intuition that this is an inherent property of a struct]

packages/jsii-diff/package.json Show resolved Hide resolved
* If present, this block indicates that an API item is no longer supported and may be
* removed in a future release. The `@deprecated` tag must be followed by a sentence
* describing the recommended alternative. Deprecation recursively applies to members
* of a container. For example, if a class is deprecated, then so are all of its members.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we propagating this in the jsii manifest? (we should)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been going back and forth on this, and ultimately decided that the assembly would map most closely to the source of truth (the source code). The propagation analysis is done by jsii-reflect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

*
* @default false
*/
subclassable?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we should separate all these flags from "docs" into some some other struct associated with each type, but I guess pragmatically we can leave them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was thinking the same, ended up with this for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also we can expose them as properties in jsii-reflect to improve discoverability...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was originally doing that, but because of the type inheritance tree in jsii-reflect it turned into copy/pasting a bunch of similar properties around and then I decided to just put everything on Docs. But yeah.

@rix0rrr rix0rrr merged commit 9cfd867 into master Apr 3, 2019
@rix0rrr rix0rrr deleted the huijbers/jsii-diff branch April 3, 2019 12:19
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

Successfully merging this pull request may close these issues.

3 participants