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

RFC: use LB3 models in an LB4 app [SPIKE - DO NOT MERGE] #2274

Open
wants to merge 15 commits into
base: master
from

Conversation

@bajtos
Copy link
Member

bajtos commented Jan 22, 2019

In this pull request, I am presenting a proof-of-concept showing how we can enable LB3 users to take their model files (JSON + JS), drop them mostly as-is into an LB4 application and get a fully working LB4 REST API mostly compatible with their own old API as provided by LoopBack 3.

The proposed compatibility layer will greatly simplify migration from LoopBack 3 to LoopBack 4. Instead of forcing users to migrate everything in one go, the new layer makes it possible to split the migration into multiple incremental steps:

  1. Create a new LB4 app with your existing LB3 models.
  • Migrate your existing test suite to the new app. Tests using supertest should be trivial to migrate. Integration/unit tests calling JavaScript API directly may require little bit more work, but hopefully few helpers to convert from app.{foo} to app.v3compat.{foo} will do most of the job.
  • Deal with breaking changes in REST API caused by incompatibilities between LB3 and LB4. This is where your test suite will come handy.
  1. Later in the future, gradually migrate LB3 models into LB4 model+repository+controller style. There is plenty of time for this, as you are no longer constrained by the deadline imposed by LB3's end-of-life.

Feedback wanted from current LoopBack 3.x users

I would like to get feedback from developers maintaining a LoopBack 3 application and looking for a way how to migrate to LoopBack 4.

  • Do you find the proposed compatibility layer useful, would you use it in your project?
  • The PoC version provides only subset of LB3 features and a lot is missing, see Implementation status. Which missing features would stop you from using the new extension to migrate your LB3 project to LB4?

/cc @strongloop/loopback-next

How to review this work

  1. Start by reviewing examples/lb3models. It's an LB4 application with LB3 models from loopback-getting-started, the example shows the user experience that we can enable using my PoC implementation.

  2. In the next step, read the architecture overview to better understand how LoopBack3 parts fit together and how they map to LoopBack 4.

  3. Take a look at Implementation status to better understand which LB3 features are supported by this spike implementation, what is missing but needed, and what is unlikely to ever work in LB4.

  4. Armed with this knowledge, it's time to start reviewing the actual TypeScript code. Please focus on high-level design and how individual classes interact together. The actual implementation is not that important right now, most of it was copied from the LB3 codebase and I expect it to change once we start to work on the real implementation.

  5. Take a look at the discussion points listed below. Do you have any opinions? Can you help me to find better answers?

Discussion points (primarily for LoopBack maintainers)

  • The PoC implementation is about 3k lines of code & docs with almost no tests. My guess/estimate is that the final production version would be somewhere between 5k and 10k lines. Are we comfortable to commit to support such large codebase for many months or even years to come?

  • How to test the code migrated LB3? Do we want to copy existing tests over? Migrate them to async/await style? Don't bother with testing the initial implementation and use few acceptance-level tests only?

  • How to split 3k+ lines of new (migrated) code into smaller chunks that will be easier to review? We can follow the steps I sort of defined by individual commits in this pull request. Can you come with a better proposals?

  • Should we register LB3 Models for dependency injection into LB4 code? Register them as repositories, models, services, or something else?

  • Should we implement booting datasources from datasources.json? I see two obstacles:

    1. Datasources must be booted before models are attached. AFAIK, it is not possible to specify the order in which the booters should be invoked, therefore we would have to add datasource booting to the single LB3 booter class.
    2. LB3 supports variable replacement in JSON config files using ENV vars but also app.get(name). This will add even more complexity to our compatibility layer and I am not sure if it's justified by the benefits.
  • Please help me find good names for the compatibility extension (@loopback/v3compat at the moment), the example app (@loopback/example-lb3models), but also the Application property exposing LB3-like app (app.v3compat right now), and so on.

  • TypeScript does not support static index signature, which make it difficult to consume custom model methods. See Microsoft/TypeScript#6480 and the comments in the PoC code. IMO, we should contribute this language feature to TypeScript as part of our work on the migration story.

  • I'd like us to extract the Booter contract into a standalone package so that v3compat and other similar extensions don't have to inherit entire boot and don't have to lock down a specific semver-major version of boot in extension dependencies. Instead, they should depend on the Booter contract only, this contract should not be changed so often (hopefully).

References

  • The user story behind this pull request: #2224
  • EPIC: Migration from LoopBack 3.x #1849

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

bajtos added some commits Jan 14, 2019

@bajtos bajtos referenced this pull request Jan 24, 2019

Merged

Improve Model type definitions #1685

1 of 1 task complete
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Jan 24, 2019

@bajtos Thank you for kicking it off with fairly comprehensive code. I think existing LB3 users will love it if we do it right.

In addition to the compat layer, should we evaluate the possibility to compose an existing LB3 app into LB4 with API explorer visibility?

@richardpringle

This comment has been minimized.

Copy link

richardpringle commented Jan 27, 2019

@bajtos, this looks pretty interesting! I'm wondering if it's not easier to require an lb3 app in it's entirety as a dependency and boot the apps together.

While the tests on a lb3 app can be ported over fairly easily, I would be concerned about my case coverage and how well I've tested all of the side effects and coupling between models in my legacy application (I would love to say that I never do this... But I suspect that I'm not alone here).

Can I expect that everything will just work the same way out of the box?

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Jan 28, 2019

@raymondfeng

In addition to the compat layer, should we evaluate the possibility to compose an existing LB3 app into LB4 with API explorer visibility?

Interesting idea, I haven't considered it before.

When using an existing LB3 app in whole, then we need to bring in loopback, strong-remoting, loopback-swagger and loopback-datasource-juggler@3 as dependencies. These modules will reach end-of-line soon, therefore such upgrade will be good for short time only and LB3 users will have to look into a different solution again. I am not sure if mounting an LB3 app inside LB4 provides enough benefits to justify two migration steps - one to mount LB3 on LB4, another to migrate to "compat" layer?

With my proposed "compat" package, upgrading users are moving completely away from the old npm modules and use LB4 counterparts instead.

  • loopback and part of strong-remoting is replaced by the new "compat" package, I expect this new package to have longer lifetime than loopback@3 et all.
  • Remaining parts of strong-remoting is replaced by @loopback/rest which I expect to be here for very long.
  • loopback-swagger is replaced by the new "compat" package too.
  • loopback-datasource-juggler@3 is replaced with loopback-datasource-juggler@4 via a dependency of the new "compat" package. If the compat package stays around long enough, I expect us to upgrade the juggler dependency to higher semver-major versions when we create them.

@richardpringle

Hey, nice to see you again :)

this looks pretty interesting! I'm wondering if it's not easier to require an lb3 app in it's entirety as a dependency and boot the apps together.

Haha, that's what @raymondfeng proposed too. Would you find such solution useful even if it does not buy you any extra time before you have to move away from LB3 anyway?

While the tests on a lb3 app can be ported over fairly easily, I would be concerned about my case coverage and how well I've tested all of the side effects and coupling between models in my legacy application (I would love to say that I never do this... But I suspect that I'm not alone here).

Can I expect that everything will just work the same way out of the box?

Kind of. Setting the known differences aside (no Bluebird API, different coercion rules at REST API layer), the rest should work the same way out of the box. Unfortunately, we cannot guarantee that - LoopBack test suite does not cover all edge cases either. We need user feedback to let us know where the compatibility layer behaves differently from LB3, so that we can incrementally and iteratively improve it.

This also allows us to spend our time on things that matter for real users, instead of trying to chase edge cases that exist in theory but nobody cares about in practice.

@richardpringle

This comment has been minimized.

Copy link

richardpringle commented Jan 28, 2019

Would you find such solution useful even if it does not buy you any extra time before you have to move away from LB3 anyway?

Absolutely useful. Sure, LB3 LTS will eventually end, but that doesn't necessarily mean that I'm going to upgrade either. There are still people out there using versions of Node that have long left LTS.

What I don't like is the idea of jumping back and forth between two different configurations where half my models are configured one way and the other half are configured another way. I would rather bundle a legacy application with say, /v1/ endpoints, and rebuild my LB3 models as LB4 models (possibly interacting with the same tables depending on their complexity).

As @raymondfeng suggests, it would also be very useful to have a unified OpenAPI spec doc, but I'm not sure how messy that would get.

@bajtos,

Take everything that I'm suggesting with a grain of salt. I use LoopBack 3 to build very small microservices where porting everything over to LB4 would not take that much work. I think that it would be beneficial to talk to someone who has a larger application to see what their opinion is.

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Jan 29, 2019

I created a new spike issue to look into mounting LB3 app in whole: #2301

@bajtos bajtos requested a review from strongloop/loopback-next Jan 31, 2019

@gtamas

This comment has been minimized.

Copy link

gtamas commented Feb 8, 2019

Hi

@bajtos

We have one LB3 app running in production right now and we would very much like to upgrade it to LB4. The ability to do this incrementally would be super nice. However, it seems to me that the current POC implementation may not provide sufficient functionality just yet.

A few issues / questions:

First, it seems the POC currently does not support remote hooks. Those are absolutely "must-haves" for us. Our code relies on them.

We also need relations to work properly. I'm not sure if the POC covers these completely. We define them in JSON and we don't really use much of the related JS APIs. Would this work with the compatibility layer?

What about authentication? We are using the built-in LB3 user model and auth token system. I know something similar (a JWT token based auth solution) is in the works for Loopback 4 right now, but I'm not sure what's the status of this and when can we expect the first stable version to be released.

What about plugins / 3rd party packages? :) We have only a few, such as softdelete mixin, pagination mixin, loopback-component-storage etc. Guess we can expect these to break, right? Or would the compatibility layer support these as well?

We are using MongoDB, but I guess that's covered by database-juggler 4.x, right?

So these are the most important features our app needs. Will the compatibility layer cover these anytime soon?
It would be really awesome to run LB3 compatibility layer with LB4 and migrate the app in parts. We definitely wouldn't want to rewrite everything in one go, especially since LB4 currently lacks a lot of functionality that LB3 provided out of the box or via plugins.

@gtamas

This comment has been minimized.

Copy link

gtamas commented Feb 8, 2019

@bajtos
@raymondfeng

By the way, obviously the compatibility layer will be pretty useful, but not only because it allows incremental migration. If it includes all the the truly essential functionality and you manage to extend its lifespan, it could also buy enough time for LB4 to "catch up".

I mean, LB4 is really great. It's a very nice architecture, but - for now at least -, it's mostly core functionality. A lot of important stuff is missing which people will have to write as extensions. The community needs time to do this.

On the other hand, LB3 already provides most of these functions / tools (either as part of its core or as some widely used 3rd party packages).

So in my opinion, running a proper compatibility layer with LB4 has additional benefits. It could provide production ready, proven solutions that people could actually use while the LB4 counterparts of these solutions are under development. As new LB4 extensions become available, the old stuff could be replaced.

Anyway, just some additional thoughts I wanted to share :)

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 12, 2019

@gtamas thank you for the feedback, I appreciate it a lot! ❤️

First, it seems the POC currently does not support remote hooks. Those are absolutely "must-haves" for us. Our code relies on them.

Noted 👍 Remote hooks were already part of my plan for the v1 release of the compat layer, see https://github.com/strongloop/loopback-next/blob/0465a94f08a973fa903f4c5400759598c1951249/packages/v3compat/README.md#missing-features-needed-for-10-release

We also need relations to work properly. I'm not sure if the POC covers these completely. We define them in JSON and we don't really use much of the related JS APIs. Would this work with the compatibility layer?

I believe relations will work. I don't have the code in place yet, but I think the implementation should be easy enough. In my plan for v1, relations are included.

What about authentication? We are using the built-in LB3 user model and auth token system. I know something similar (a JWT token based auth solution) is in the works for Loopback 4 right now, but I'm not sure what's the status of this and when can we expect the first stable version to be released.

Ah, this is tricky. The way how I am envisioning the compatibility layer, there will be no built-in models for authorization - but you may be able to copy them over from the old LoopBack. We will need to look into ways how to plug LB3 AccessToken into LB4 authentication & authorization mechanisms. Those mechanisms are not fully fleshed out yet and we don't have any ETA, the work is going slower that we anticipated.

What about plugins / 3rd party packages? :) We have only a few, such as softdelete mixin, pagination mixin, loopback-component-storage etc. Guess we can expect these to break, right? Or would the compatibility layer support these as well?

I think it should be possible to support mixins. We may need a bit of research to find out how to load & register them in the same way as LB3 does, but I think it will be doable.

As for loopback-component-storage, it's working more like a connector than a typical component, isn't it? I mean it's configured via datasources.json and model-config.json, not via component-config.json. If that's the case, then it should work.

We are using MongoDB, but I guess that's covered by database-juggler 4.x, right?

Yes 🎉

Will the compatibility layer cover these anytime soon?

Honestly, the compatibility layer is a lot of work, my rough estimate is that it will take months to complete. Would you like to join the effort and contribute some parts yourself?

For near-term, we are planning to start with a slightly different approach that will be faster to implement, see #2318.

@b-admike
Copy link
Member

b-admike left a comment

Lots of code to review (not super familiar with strong-remoting and other areas of compat layer), but I get the gist of the spike and I like how well you've put it together and the direction we are heading. Especially after reading the architectural overview, the design and mapping sounds reasonable 👍.

expect(todo.getId()).to.equal(1);
});

it('provides Model.app.models.AnotherModel API', () => {

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

provides Model.app.models.Model API here?

export function setupPersistedModelClass(
registry: Lb3Registry,
): typeof PersistedModel {
const ModelCtor = registry.getModel('Model');

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

Question: is the Model string the name for the default Model base class here?

// TODO: use the implementation from LB3's lib/application.js
const ds = this.registry.createDataSource(name, config);
this.dataSources[name] = ds;
return ds;

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

Do we need to return the created datasource here? I thought we were adding a datasource definition to the app (like app.model below).

lb3api.md Outdated
1. expose remote methods via REST
2. boot
Should we register LB3 Models for dependency injection into LB4 code? Register
them as repositories, models, services, or something else?

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

Good questions; LB3 Datasources were easy to register for DI into LB4 app, but LB3 models are tricky. IMO LB3 models are synonymous with LB4 repositories or services.

lb3api.md Outdated

## Model
- https://github.com/Microsoft/TypeScript/issues/6480
- extract the Booter contract into a standalone package so that v3compat does

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

Can you explain what 'Booter contract' is in this case? EDIT: I kind of have an idea what it is (i.e. a booter class).

```

IMPORTANT! These files must live outside your `src` directory, out of sight
of TypeScript compiler.

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

Should we say why here?


Remove `_meta` section, LoopBack 4 does not support this config option.

4. Modify your Application class and apply `CompatMixin`.

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

nitpick: might want to mention filename here

}
```

6. Register your legacy datasources in Application's constructor.

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

To clarify, at this point, our compat lb3-model booter would have configured and loaded the legacy models for us right? How do we attach legacy datasource(s) to a legacy model after?

@@ -16,7 +16,8 @@ const fileExists = promisify(fs.exists);
const debug = debugFactory('loopback:v3compat:model-booter');

const DefaultOptions = {
root: './legacy',
// from "/dist/src/application.ts" to "/legacy"
root: '../../legacy',

This comment has been minimized.

@b-admike
## Open questions

- How to test code migrated from strong-remoting and loopback (v3)? Do we want
to copy existing tests over? Migrate them to async/await style? Don't bother

This comment has been minimized.

@b-admike

b-admike Feb 14, 2019

Member

IMO the answer ideally should be definitely use async/await style, but re-write them using the tests for inspiration. If you were to reflect on how you migrated strong-remoting and loopback (v3) code over for the spike, do you feel that there were significant changes to warrant a re-write of the tests?

@b-admike

This comment has been minimized.

Copy link
Member

b-admike commented Feb 14, 2019

  • The PoC implementation is about 3k lines of code & docs with almost no tests. My guess/estimate is that the final production version would be somewhere between 5k and 10k lines. Are we comfortable to commit to support such large codebase for many months or even years to come?

This IMO is dependent on how fast we can get the compat layer up and running for users to migrate from LB3 to LB4 while we bridge the feature gaps in LB4. From this PoC, I feel like this makes migration easier and can be hopefully done in months and not years so that it gives way to smooth migration and allows us to work on feature parity in the meantime.

  • How to split 3k+ lines of new (migrated) code into smaller chunks that will be easier to review? We can follow the steps I sort of defined by individual commits in this pull request. Can you come with a better proposals?

I think the commits follow a logical progression, but if we can use the architectural overview to break them up (like how the sections are there), then it'd be awesome.

  • Should we implement booting datasources from datasources.json? I see two obstacles:

    1. Datasources must be booted before models are attached. AFAIK, it is not possible to specify the order in which the booters should be invoked, therefore we would have to add datasource booting to the single LB3 booter class.
    2. LB3 supports variable replacement in JSON config files using ENV vars but also app.get(name). This will add even more complexity to our compatibility layer and I am not sure if it's justified by the benefits.

This could possibly be a spike story to investigate those two challenges.

  • Please help me find good names for the compatibility extension (@loopback/v3compat at the moment), the example app (@loopback/example-lb3models), but also the Application property exposing LB3-like app (app.v3compat right now), and so on.

I don't have better suggestions at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment