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) refactored to allow route templates to be overridden #81

Closed

Conversation

cadriel
Copy link
Contributor

@cadriel cadriel commented Mar 16, 2017

First stab at refactoring the way route templates are handled.

Note: I'm very tempted to move the hardcoded handlebar content in routeGenerator.ts on around line: 50 to the templates too, but wanted your thoughts first @lukeautry.

Otherwise - all tests are passing and this seems to be working ok.

@cadriel
Copy link
Contributor Author

cadriel commented Mar 16, 2017

Encountered a few pathing issues - sorting them out.

@cadriel cadriel force-pushed the feature/customRouteTemplates branch from 1262dc2 to 5967a31 Compare March 16, 2017 23:29
@cadriel
Copy link
Contributor Author

cadriel commented Mar 17, 2017

Other than my comment re: require, I think this is good to go.

Copy link
Contributor Author

@cadriel cadriel left a comment

Choose a reason for hiding this comment

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

fwiw - I have it running locally with this code;

    const env = process.env.NODE_ENV;
    console.error('Environment', env);
    let canImportByAlias = true;
    if (env === 'test') {
      canImportByAlias = false;
    }

Seems to produce the same result but also allows you to run with a linked npm package..

let canImportByAlias: boolean;
try {
require('tsoa');
require.resolve('tsoa');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels problematic if you're trying to test with a project that is linked locally to tsoa. I tested, and if using link - require always throws an error.

If this is only there to support the tests running - could we perhaps use NODE_ENV instead to set the environment to test - and change the imports that way instead?

@cadriel cadriel force-pushed the feature/customRouteTemplates branch from 5967a31 to 68eaffb Compare March 17, 2017 14:57
@cadriel
Copy link
Contributor Author

cadriel commented Mar 17, 2017

Final update;

  • Moved all template code to the template files, no more concat.
  • Added handlebars-helpers. For those that want to have basic comparison operators in their templates.
  • Changed to the code above whereby we check environment rather than availability of a module.

Also ran yarn to update the lock file.

@lukeautry
Copy link
Owner

@cadriel Thanks for doing this work! We've needed something like this for a while.

One issue with the PR right now is the use of NODE_ENV=test in Windows. I think maybe we can use something like https://github.com/kentcdodds/cross-env to get the same effect in a cross-platform way.

@cadriel
Copy link
Contributor Author

cadriel commented Mar 20, 2017

Sure, np. :)

I'll push in an update that makes use of cross-env momentarily.

I also have some more work that reduces the amount of code produced in the routes.ts - and instead defer's it to two middleware functions. One handles the parameter checking in the same way (but passes the validated args in the request) - the other loads the controller by way of a factory service.

Not quite ready yet, but if you were interested in those changes I could push them in too when ready.

@cadriel cadriel force-pushed the feature/customRouteTemplates branch from 68eaffb to 145d611 Compare March 20, 2017 02:37
@cadriel
Copy link
Contributor Author

cadriel commented Mar 20, 2017

Updated to use cross-env.

@lukeautry
Copy link
Owner

Thanks @cadriel . Sure, I'll wait to merge until those changes come up.

@cadriel
Copy link
Contributor Author

cadriel commented Mar 20, 2017

Looks like there's still some issues building on Windows. I'll look into these and let you know once I have it running there.

@cadriel
Copy link
Contributor Author

cadriel commented Mar 20, 2017

Ok @lukeautry;

This is now building and testing fine in both MacOS and Windows 10 for me.

…ss-env to ensure cross platform compatability
@cadriel cadriel force-pushed the feature/customRouteTemplates branch from a03b731 to 1440707 Compare March 20, 2017 03:16
@cadriel
Copy link
Contributor Author

cadriel commented Mar 20, 2017

You can probably merge this one now if you're happy with it.

The other work I mentioned above I'll create another PR for in the future.

@cserron
Copy link

cserron commented Mar 25, 2017

Hi,
When do you think this will be ready to be merged in?
We are actually waiting for something like this to be implemented in our TSOA project.
Thanks.

@lukeautry
Copy link
Owner

lukeautry commented Mar 25, 2017 via email

@lukeautry
Copy link
Owner

Merged, released in 1.1.7. Thanks @cadriel!

@lukeautry lukeautry closed this Mar 26, 2017
@cadriel
Copy link
Contributor Author

cadriel commented Mar 27, 2017

np :)

@cadriel cadriel deleted the feature/customRouteTemplates branch March 30, 2017 21:41
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.

None yet

3 participants