apidom icon indicating copy to clipboard operation
apidom copied to clipboard

Replace stampit with TypeScript mixins

Open char0n opened this issue 1 year ago • 8 comments

The goal of this issue is to determine if it make sense to switch from stamps to mixins. If it is determined that we're going to do that, another issue should be created to track the implementation progress there.

Pragmatically it just make sense to be able to use advanced TypeScript 5 features like decorators. Let's push through with this on OpenAPI 2.0 namespace, and see where we'll get.


Resources:

  • https://medium.com/@koresar/fun-with-stamps-episode-16-typescript-mix-in-classes-vs-stamps-5e3c548753ea
  • https://www.typescriptlang.org/docs/handbook/mixins.html

char0n avatar Nov 30 '23 16:11 char0n

Additional notes:

After further research I've come into the conclusion (subjective one), that It's currently really not worth the effort. If we would like to switch to TypeScript mixins we would have to rewrite significant amount of code and we'd still would have to use some kind of abstraction like ts-mixer to provide convenient API.

We would replace one way of doing things (IMHO more flexible with stamps) with another one, which is less flexible.

char0n avatar Jan 04 '24 13:01 char0n

Thinking further...what we can do is to identify packages where using stamps is not really necessary. We really only need stamps for facilitating multiple inheritance which is not natively supported by JavaScript/TypeScript Classes - we use stamps mostly in namespace packages = packages that starts with apidom-ns-*

So for example this package: https://github.com/swagger-api/apidom/tree/main/packages/apidom-parser could be transformed to TypeScript Classes (with proper visibility).

It will create a breaking change as instead of const parser = ApiDOMParser(); we'll have to do const parser = new ApiDOMParser(); but it's still acceptable as we're in pre-release mode

We can even avoid the breaking change by utilizing the pattern for instantiating a class without a new keyword.

char0n avatar Jan 04 '24 13:01 char0n

Another package that has been identified is https://github.com/swagger-api/apidom/tree/main/packages/apidom-ast

All the AST node types can be represented as TypeScript classes as there isn't multiple inheritance involved.

char0n avatar Jan 10 '24 07:01 char0n

The packages that I've been working on:

  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-parser
  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-parser-adapter-json
  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-parser-adapter-yaml-1-2
  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-ast

I'll make the suggested changes from https://github.com/swagger-api/apidom/pull/3646 to Parser and to the other packages, as some cases will apply there as well. The Adapter packages should be ready for a PR then. After that, I'll continue the work on the AST package.

I believe that these packages can be changed as well:

  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-core
  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-reference
  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-ls - only one case here

glowcloud avatar Jan 10 '24 08:01 glowcloud

Status update for each of the previously mentioned packages:

Changes merged into main:

  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-parser
  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-parser-adapter-json
  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-parser-adapter-yaml-1-2

Changes waiting for a PR review:

  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-ast - #3681

Currently in progress:

  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-core

To change:

  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-reference
  • https://github.com/swagger-api/apidom/tree/main/packages/apidom-ls

glowcloud avatar Jan 16 '24 13:01 glowcloud

We already bumped into issue of breaking the backward compatibility. To avoid breaking the compatibility mainly with (swagger-client) we would utilize following strategy for the time being:

[!IMPORTANT]
Let's convert what we can without introducing breaking changes. Let's note down things that cannot be converted; and continue until we get into stage that we cannot do anymore changes without going for 1.0.0-alpha.0. Going for 1.0.0-alpha.0 means we have to amend the monorepo processes and do significant changes in swagger-client. We have to be diligent to document in this issue what was skipped due to breaking compatibility. We can do even better and use special TODO comments inside the code in following form:

// @TODO(<developer-email>): transforming this stamp to class will break backward compatibility

When we are in this phase, that we converted every stamp to class that could be converted without breaking the backward compatibility, next steps would be as follows:

We will progressively introduce stamp -> class transformation which will cause breaking changes for swagger-client. To avoid breaking swagger-client and other swagger tooling we have to utilize special versioning strategy. We would have to go for 1.0.0-alpha.0 - that will allow us to still stay withing pre-development phase ( as we currently are in 0.x.y) but will not influence other swagger tooling as we've used version matching >=0.90.0 <1.0.0 that is outside of 1.0.0; this basically means that swagger tooling will not automatically install 1.0.0-alpha.x releases, which is what we want. swagger-client would need to be manually adapter to work with 1.0.0-alpha.x releases, but already released npm packages will still work properly.

char0n avatar Jan 19 '24 09:01 char0n

Work is currently being done on apidom-ns-workflows-1 package.

Withing the package there is one stamp that's being composed to new stamp. We will do the following conversion to get rid of stampit explicit dependcy:

From:

const JSONSchemaVisitor = stampit(SchemaVisitor, {
  props: {
    specPath: always(['document', 'objects', 'JSONSchema']),
    canSupportSpecificationExtensions: false,
    jsonSchemaDefaultDialect: 'https://json-schema.org/draft/2020-12/schema',
  },
  init() {
    this.element = new JSONSchemaElement();
  },
}); 

To:

const JSONSchemaVisitor = SchemaVisitor.compose({
  props: {
    specPath: always(['document', 'objects', 'JSONSchema']),
    canSupportSpecificationExtensions: false,
    jsonSchemaDefaultDialect: 'https://json-schema.org/draft/2020-12/schema',
  },
  init() {
    this.element = new JSONSchemaElement();
  },
});

This will avoid using import of stampit directly. Once we transform the stamp to class in dependenct package @swagger-api/apidom-ns-openapi-3-1 , apidom-ns-workflows-1 will report error in tests and we'll have to compensate.

Currently this seems like the simplest approach. We would start with namespace packages that are not used as dependency to other packages.

char0n avatar Jan 23 '24 12:01 char0n

API Design Systems namespaces has been addressed as first namespace package and as a POC in https://github.com/swagger-api/apidom/pull/3704. It didn't have any other namespace packages as dependencies so it was simpler to deal with.

char0n avatar Jan 23 '24 12:01 char0n

Current plan is to finish this, create breaking changes, fully get rid of stamps and release v1.0.0-alpha.1

char0n avatar May 09 '24 10:05 char0n

Finalized in https://github.com/swagger-api/apidom/pull/4105

char0n avatar May 14 '24 08:05 char0n