Stricter extension name typing, changed build system and test system #80

jonluca · 2022-08-07T19:52:36Z

Version 2.1.0

2022.08.07

Updated libs
Changed build system to use esbuild
Changed test system to use vite
Added stricter extension prefix typing

…to use esbuild and vite

jonluca · 2022-08-07T19:53:02Z

src/model/SpecificationExtension.ts

-    // Cannot constraint to "^x-" but can filter them later to access to them
-    [extensionName: string]: any;
-}
+export type IExtensionName = `x-${string}`;


With newer versions of typescript you can actually strongly type a string prefix, which is nice

jonluca · 2022-08-07T19:53:21Z

build.mjs

+const entryPoints = glob
+    .sync('src/**/*.ts', { root: path.join(__dirname, 'src') })
+    .filter((l) => !l.includes('spec.ts'));
+await esbuild.build({


This is significantly faster than tsc (~50x for this project)

jonluca · 2022-08-07T19:53:51Z

vite.config.ts

@@ -0,0 +1,18 @@
+import { defineConfig } from 'vitest/config';
+
+export default defineConfig({


vitests are significantly faster than mocha - they all complete in ~500ms

pjmolina

Hi @jonluca:

First of all, thank you very much for your contribution & time devoted.

Looking your PR, I see it can be splitted in 3 different parts, I want to comment about them independently:

Improving the type checking for x-* extensions point.
Using a new framework for tests: vitest instead of mocha.
Using a new build framework esbuild instead of tsc.

My comments on them:

For the first one. Its a clear improvement. We can incorporate it. Now the TS type system in newer versions is more expressive for cases like this one. Thank you for the improvement.
Using vitest instead of mocha is more a question of taste. Gaining some seconds when running tests is only a marginal gain (compared to forcing users/maintainers to use/learn a new tool/framework). Mocha has many plugins, once I was using was mocha-teamcity-reporter that was lost on your PR. Maybe can achieve same missing funtionality with vitest-teamcity-reporter. Anyway not against the usage of vitest, just pondering the pros & cons as selecting development frameworks always has.
Using a new build framework. esbuild. I read a bit about its documentation: esbuild basically erase the type info with a explicit NO-TYPE-CHECKING (see https://esbuild.github.io/content-types/#typescript). And you still need to use tsc -noEmit to check the types. Your bet here is about improving build times (x50 accordingly to your report).
My main concern here is to preserve the library as close as possible to TSC to preserve backward compatibility and do strict type checking. A great majority of openapi3-ts clients are consuming it directly from Typescript.

So a faster build here is only marginal gain for maintainers when building the JS version (build time before publishing the lib).
TS users will use TS files from the library (if they want to, included in the redistribution package) so they can build/bundle with any tool (tsc, esbuild or other: not opinionated here).
Also esbuild does not support: "Features that need a type system are not supported" (emitDecoratorMetadata & declaration) from docs. In the roadmap for the library, having metadata and providing decorators are a feature I want to dig in.
So, for the reasons just explained, I feel keep using tsc is a safer and more convenient, for the moment, for long term support than jumping into the esbuild wagon for now. I am totally OK is the price to pay here is library build speed.

Looking forward to hear your comments about it @jonluca
So far, I am ready to merge PRs for (1) and (2) and postponing (3) if we both agree on that.

Thank you!

jonluca · 2022-08-08T14:43:04Z

Hi @pjmolina - that sounds very reasonable.

I agree with all your points. Let me add in the vitest-teamcity-reporter to preserve the old functionality.

I've updated the diff removing esbuild and only using tsc.

One question - I can't seem to find the travis build anywhere, https://www.travis-ci.com/metadevpro/openapi3-ts just 404s

jonluca · 2022-08-08T14:43:50Z

I also created ESM and CJS versions of the package

pjmolina · 2022-08-08T15:18:32Z

Excellent, thank you.
Need to remove or reconfigure Travis. It was lost when moving from .org to .com.
I will remove it from the moment.

pjmolina · 2022-08-08T15:31:42Z

src/dsl/index.ts

@@ -1 +1 @@
-export * from './OpenApiBuilder';
+export * from './OpenApiBuilder.js';


Do you really need the .js extension for any use case?

I was following the TS convention of avoiding extensions on imports to refer TS/JS files depending on the context (edit time)/(build time).

This prevents issues with module resolution in node ESM exports. It's very annoying, I agree

https://stackoverflow.com/questions/62619058/appending-js-extension-on-relative-import-statements-during-typescript-compilat

More context there, but for now this is the simplest fix.

Thanks for the explanation. Agreed, let's keep the extensions.

pjmolina · 2022-08-08T15:36:55Z

tsconfig.json

-    "removeComments": true,
-    "preserveConstEnums": true,
-    "noEmitHelpers": true,
-    "outDir": "dist",


Changing the outDir=dist in favor of dist/mjs and dist/cjs is a good improvement, but also a breaking change.
We can publishing it as 3.0.0 to warn consumers and preserve SemVer semantics.

If you can change the version number, please. I think we are all done and ready to merge.

Sounds good!

A nice script around this is semantic-release - you can see an example here https://github.com/jonluca/har-to-openapi/blob/main/.github/workflows/release.yml

It's nice because you never have to update package.json yourself, and it deploys automatically based on your commits. Not saying we need to include it here, but something nice to keep in mind.

Updated the diff with 3.0.0

Good point, thanks!

pjmolina

Looks great to me.
Thanks @jonluca
Merging!

Stricter extension name typing, changed build system and test system …

511d21e

…to use esbuild and vite

jonluca commented Aug 7, 2022

View reviewed changes

pjmolina self-requested a review August 8, 2022 14:25

pjmolina requested changes Aug 8, 2022

View reviewed changes

feat(build): removed esbuild, added cjs and esm exports

df8d6a0

feat(build): removed esbuild, added cjs and esm exports

c0fbb27

pjmolina reviewed Aug 8, 2022

View reviewed changes

feat(version): bump version number

181cc68

pjmolina approved these changes Aug 8, 2022

View reviewed changes

pjmolina merged commit 7a25a0c into metadevpro:master Aug 8, 2022

pjmolina mentioned this pull request Aug 16, 2022

v3.0: __exportStar is not defined #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stricter extension name typing, changed build system and test system #80

Stricter extension name typing, changed build system and test system #80

jonluca commented Aug 7, 2022

jonluca Aug 7, 2022

jonluca Aug 7, 2022

jonluca Aug 7, 2022

pjmolina left a comment

jonluca commented Aug 8, 2022

jonluca commented Aug 8, 2022

pjmolina commented Aug 8, 2022

pjmolina Aug 8, 2022

jonluca Aug 8, 2022

pjmolina Aug 8, 2022

pjmolina Aug 8, 2022

pjmolina Aug 8, 2022

jonluca Aug 8, 2022

pjmolina Aug 8, 2022

pjmolina left a comment

		@@ -0,0 +1,18 @@
		import { defineConfig } from 'vitest/config';

		export default defineConfig({

		@@ -1 +1 @@
		export * from './OpenApiBuilder';
		export * from './OpenApiBuilder.js';

Stricter extension name typing, changed build system and test system #80

Stricter extension name typing, changed build system and test system #80

Conversation

jonluca commented Aug 7, 2022

Version 2.1.0

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjmolina left a comment

Choose a reason for hiding this comment

jonluca commented Aug 8, 2022

jonluca commented Aug 8, 2022

pjmolina commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjmolina left a comment

Choose a reason for hiding this comment