-
Notifications
You must be signed in to change notification settings - Fork 229
Add ts support #920
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
base: develop
Are you sure you want to change the base?
Add ts support #920
Conversation
integration test code coverage
Coverage Breakdown • (19%)
|
unit test code coverage
Coverage Breakdown • (88%)
|
…nto feature/ts-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds TypeScript support to the project by introducing type definitions and converting the main entry point to TypeScript. The changes enable better type safety and developer experience when using this OpenAPI to Postman converter library.
Key changes:
- New TypeScript type definitions in
types/index.tscovering all major interfaces - Conversion of
index.jstoindex.tswith TypeScript annotations - Build pipeline setup with TypeScript compilation to
dist/directory
Reviewed changes
Copilot reviewed 25 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| types/index.ts | Comprehensive type definitions for converter inputs, options, and outputs |
| index.ts | TypeScript conversion of main entry point with type annotations |
| package.json | Added TypeScript tooling, build scripts, and updated main/types entry points |
| libV2/index.js | Updated import paths to reference reorganized CollectionGeneration directory |
| libV2/CollectionGeneration/schemaUtils.js | Added missing eslint-disable comment and fixed import paths with .js extensions |
| libV2/CollectionGeneration/validationUtils.js | Updated import paths with .js extensions for relocated modules |
| test/unit/*.test.js | Updated test imports to use compiled dist/index.js instead of source |
| test/system/*.test.js | Updated test imports to use compiled dist/index.js |
| test/integration/integration.test.js | Updated import to use compiled dist/index.js |
| scripts/test-lint.sh | Removed index.js from eslint targets since it's now TypeScript |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { MODULE_VERSION } = require('../lib/schemapack.js'); | ||
| const SchemaPack = require('../lib/schemapack.js').SchemaPack; | ||
| const UserError = require('../lib/common/UserError'); |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The require paths use '../lib/' but should use './lib/' since index.ts is at the project root, not in a subdirectory. These incorrect paths will cause module resolution failures at runtime.
| const { MODULE_VERSION } = require('../lib/schemapack.js'); | |
| const SchemaPack = require('../lib/schemapack.js').SchemaPack; | |
| const UserError = require('../lib/common/UserError'); | |
| const { MODULE_VERSION } = require('./lib/schemapack.js'); | |
| const SchemaPack = require('./lib/schemapack.js').SchemaPack; | |
| const UserError = require('./lib/common/UserError'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 28 out of 38 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { MODULE_VERSION } = require('../lib/schemapack.js'); | ||
| const SchemaPack = require('../lib/schemapack.js').SchemaPack; | ||
| const UserError = require('../lib/common/UserError'); |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The require statements use relative paths starting with '../lib/' but these files are now at './lib/' relative to index.ts in the project root. These paths will fail at runtime since the compiled JavaScript will be in the dist directory.
| const { MODULE_VERSION } = require('../lib/schemapack.js'); | |
| const SchemaPack = require('../lib/schemapack.js').SchemaPack; | |
| const UserError = require('../lib/common/UserError'); | |
| const { MODULE_VERSION } = require('./lib/schemapack.js'); | |
| const SchemaPack = require('./lib/schemapack.js').SchemaPack; | |
| const UserError = require('./lib/common/UserError'); |
| return schema.detectRelatedFiles(); | ||
| }, | ||
|
|
||
| bundle: async function(input: MultiFileSpecInput & { options?: Options }): Promise<BundledContent> { |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter type uses intersection with an inline type which is inconsistent with the defined MultiFileSpecInput interface. Consider adding an 'options' field directly to MultiFileSpecInput or creating a separate BundleInput type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to move this ?
| const utils = require('./utils.js'); | ||
|
|
||
| const schemaFaker = require('../assets/json-schema-faker'), | ||
| const schemaFaker = require('../../assets/json-schema-faker.js'), | ||
| _ = require('lodash'), | ||
| mergeAllOf = require('json-schema-merge-allof'), | ||
| xmlFaker = require('./xmlSchemaFaker.js'), | ||
| xmlFaker = require('../xmlSchemaFaker.js'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wouldn't require extensions for js files
|
|
||
| it('.gitignore coverage must be a subset of .npmignore coverage', function () { | ||
| expect(_.intersection(gitignore, npmignore)).to.eql(gitignore); | ||
| it('.gitignore coverage must be a subset of .npmignore coverage (except dist which is built for npm)', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files ignored by git should ideally also be ignored by npm, except dist/ files which needs to be published to npm
| @@ -0,0 +1,187 @@ | |||
| /** | |||
| * Conversion options | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a selective version of options.js with types or exhaustive ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we now use this options the api-spec module as well ?
Lets expose d.ts for this ?
|
are we not creating |
| preferredRequestBodyType?: 'x-www-form-urlencoded' | 'form-data' | 'raw' | 'first-listed'; | ||
| } | ||
|
|
||
| export type SpecInput = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets call it Specification ?
| export type ConversionCallback = ( | ||
| err: { message: string; name?: string } | null, | ||
| result?: ConversionResult | ||
| ) => void; | ||
|
|
||
| interface MetadataResult { | ||
| result: true; | ||
| name: string; | ||
| output: { type: 'collection'; name: string }[]; | ||
| } | ||
|
|
||
| export type MetadataCallback = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make this generic and not add specific type for each type of callback
| origin?: 'browser'; | ||
| } | ||
|
|
||
| export interface MultiFileSpecInput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
…nto feature/ts-support
No description provided.