Distinguish between "file" and "File" result types

[fabien]

If you have a Loopback model called File you'll receive following error when it is set as a return type: Error: Cannot create a file response from object

In other words, because file is now a reserved keyword with a very specific response handling logic, and File is lowercased during normalization into resultType, the error above would prevent any Loopback being named File. This introduced a breaking change for my apps.

This is loosely related to #318 but in the above case, it's actually not desirable to return a file (buffer/stream) response. Perhaps a more specific keyword than simply file would have been better in hindsight.

See also: https://github.com/strongloop/strong-remoting/commit/a85068ef1122751efbbf3e9b2a1b9439ae70fa3f#commitcomment-19410422

@bajtos @richardpringle

[fabien]

Related (more or less duplicate of): https://github.com/strongloop/loopback/issues/2554

[bajtos]

@fabien thank you for the pull request. I am ok to land this patch as short-term fix.

For longer term: I think the situation where type: 'file' is something else than type: 'File' is quite confusing. Let's discuss how can we improve the format of remoting metadata in https://github.com/strongloop/loopback/issues/2554

[fabien]

@bajtos thanks for the quick reply and aim to land it ASAP! With strong-remoting being a dependency of loopback a short-term fix for me would have involved forking both in order to fix the package.json referencing my fork on Github ... much appreciated.

Will look into the discussion some more.

[bajtos]

@fabien there are few more places where we are dealing with type: 'file', see e.g. https://github.com/strongloop/strong-remoting/blob/1efefa62d5e4c393ffe631e560e1946f688acb24/lib/shared-method.js#L417, https://github.com/strongloop/strong-remoting/blob/1efefa62d5e4c393ffe631e560e1946f688acb24/lib/shared-method.js#L567 and https://github.com/strongloop/strong-remoting/blob/1efefa62d5e4c393ffe631e560e1946f688acb24/lib/shared-method.js#L578. PTAL.

[fabien]

@bajtos as you noted here:

https://github.com/strongloop/strong-remoting/blob/2.x/lib/http-context.js#L415

Why aren't we using convertToBasicRemotingType? If we were, everything could be centralized by internally prefixing the type (example: #file or $file), no?

[fabien]

@bajtos I have gone ahead and used convertToBasicRemotingType and then prefix/normalize actual file types as res:file (meaning reserved or response). We can use a different prefix if you want. With this change, handling such custom types is now much more generic. If you wanted to be explicit in your remote method declaration, you could add the prefix yourself:

{ type: 'res:file' }

And it would still work as expected. This means it's backwards compatible, because file, File and res:file will all be handled appropriately.

[bajtos]

@fabien I have a different opinion here. I would rather not add a new set of type strings like res:file, I am worried it will create even more confusion (why there is object and not res:object for example?).

As I commented in https://github.com/strongloop/loopback/issues/2554#issuecomment-253786224, I think we should simply make File a reserved type starting from 3.0 release, in which case this fix will stay in the 2.x branch only.

Anyhow, I went ahead and simplified/cleaned up your patch in 7691ff7 to a state that looks good to myself. Could you please take a look at the resulting change? (I pushed the commit to your feature branch thanks to a recently added GitHub feature described in https://github.com/blog/2247-improving-collaboration-with-forks).

If you are ok with what we have here now, then either me or you can squash the commits into a single one, provide a descriptive commit message per our contributor docs, and this pull request can be landed.

[fabien]

@bajtos thanks for the cleaned-up patch - I'm totally fine with your implementation! If you don't mind, please go ahead and squash & pull to get it landed 👍

[fabien]

Only concern would be the 3.0 release upgrade path for models named File, but I guess that's unavoidable.

[bajtos]

Only concern would be the 3.0 release upgrade path for models named File, but I guess that's unavoidable.

Yes, I do realise this means users will have to rework their apps a bit when upgrading from 2.x to 3.0. As you said, this is unavoidable :(

[bajtos]

Landed and released in strong-remoting@2.32.2, thank you for the contribution!

[fabien]

@bajtos thanks for landing it so quickly!