j-d-carmichael / boats

Beautiful Open Api Template System
MIT License
57 stars 8 forks source link

Using "paths" in boatsrc to define absolute paths that can be used in your spec. #51

Closed TomFreeman closed 3 years ago

TomFreeman commented 3 years ago

A proof of concept demonstrating using a simple shorthand (in this case $) to refer to the root of the spec repository.

The intention is to improve the developer experience by removing the need to manage relative paths where content can move independently (e.g. between paths, schemas and mixins).

If the concept is okay, can improve the testing suite to improve coverage and make tests more explicit.

TomFreeman commented 3 years ago

For some additional context, we have a common "base" mixin that is applied to all files and does some basic things like custom tagging and generating operation IDs.

This at least makes copy/pasting that block manageable, and could be built on to apply mixins automatically similar to the inject functionality.

j-d-carmichael commented 3 years ago

Ah that is very nice! I'll check the code out in detail ASAP. With this in place it could warrant a new or addition to an IDE yaml extension - the previous work around was to reference the compiled reference, eg

$ref: #/some/definition

But this is way cleaner, great idea!

Is $ the correct character though? I spend my life in typescript these days and become very used to the @ char being a shorthand ref. and it being configured in the tsconfig file.

eg

{
  "compilerOptions": {
    "module": "commonjs",
    "esModuleInterop": true,
    "target": "ES2017",
    "moduleResolution": "node",
    "noImplicitAny": true,
    "sourceMap": true,
    "outDir": "build",
    "baseUrl": "./",
    "resolveJsonModule": true,
    "importHelpers": true,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "allowSyntheticDefaultImports": true,
    "paths": {
      "@database": [
        "src/database"
      ],
      "@/*": [
        "src/*"
      ]
    },
    "plugins": [
      {
        "transform": "@zerollup/ts-transform-paths",
        "exclude": ["*"]
      }
    ]
  },
  "exclude": [
    "package.json"
  ]
}
TomFreeman commented 3 years ago

Is $ the correct character though? I spend my life in typescript these days and become very used to the @ char being a shorthand ref. and it being configured in the tsconfig file.

I'm absolutely not wedded to the '$' character, just wanted to avoid '#' as that has a clear meaning already.

j-d-carmichael commented 3 years ago

@p-mcgowan what is your thoughts on this one?

With this in place we could also adopt a similar tsconfig paths object in the .boatsrc file. That would become useful when re-using model defs in other APIs eg:

"paths": {
  "@common": [
    "../common-model-defs"
  ],
  "@/*": [
    "src/*"
  ]
},
p-mcgowan commented 3 years ago

Yeah sounds cool, would be a good option for files that are not referenced in an index file.

TomFreeman commented 3 years ago

This might only affect the test cases, but should paths described in the .boatsrc file be relative to the .boatsrc file itself, or to the base index file (the one with the openapi/asyncapi root definition.)

I started my implementation assuming they should be relative to the .boatsrc file, but for reasons it might be easier to use the root spec file.

j-d-carmichael commented 3 years ago

I would say from the .boatsrc file as this follows the convention set by typescript with the tsconfig file.

p-mcgowan commented 3 years ago

Depends though - if you say npx boats -i ./some/deeply/nested/folder then it would make sense that the root of the swagger project is the input path (ie some/deeply/nested/folder) as opposed to where the root of the project is.

j-d-carmichael commented 3 years ago

maybes.. buutt you're always going to have to run it from where your rc file is, are the deeply nested projects are that common?

TomFreeman commented 3 years ago

This is now a "proper" implementation open for comments.

Thanks.

TomFreeman commented 3 years ago

@p-mcgowan / @j-d-carmichael I'll be AFK for a week or so. Happy for someone to take this over or can leave it till I'm back.

Unless you think it's fine as it is, in which case 🥳

j-d-carmichael commented 3 years ago

I had a read over everything already and it all looks good to me. Want to give it closer inspection before releasing though when i find a moment :)

p-mcgowan commented 3 years ago

Yeah I meant to give it a run through a few projects but didn't find the time - will probably give it a proper read-over and spin this weekedn

j-d-carmichael commented 3 years ago

Tom I checked out the PR to give it a test run but hit an error.

File: srcOA2/paths/v1/weather/latest/get.yml.njk Line: 12

Change it from: $ref: ../../../../definitions/weather/models.yml.njk to:
$ref: @/srcOA2/definitions/weather/models.yml.njk

ResolverError: Error opening file "somepath... /opensource/boats/test-build/srcOA2/definitions/weather/models.yml

I get the same error with or without the .njk file ext. Maybe I missed something in the config?

TomFreeman commented 3 years ago

Thanks. Ah, yes, there's a problem using the root of the workspace, in that the absolute source paths aren't re-written over to templated paths.

So, it's still trying to find the templated file, but in the source directory, not in the directory it's templated to.

You're okay if you are (as I was) defining absolute paths within your source, because they're re-written to relative paths. Will have to think about that, because it's a legitimate thing a user will do, and the principle of least surprise should apply.

TomFreeman commented 3 years ago

@j-d-carmichael I think I've fixed that issue now, but could probably do with some more test cases.

But please have a look at the fix, see if it makes sense and I'll flesh out the examples later.

j-d-carmichael commented 3 years ago

Cool I'll try and check it out tomorrow morning 👍

j-d-carmichael commented 3 years ago

@TomFreeman this looks great! Just gave it a few test runs and all seems to be working ok.

We noticed the docs on gh-pages was getting neglected last week so moved to a sub dir: https://github.com/liffery-com/boats/blob/master/docs/README.md Would be great if you add a couple of notes about your new feature too :+1:

j-d-carmichael commented 3 years ago

:D sorry @TomFreeman seems after changing the default branch to main from master delete this PR - if you point it to develop though should be all good