RFC: use LB3 models in an LB4 app [SPIKE - DO NOT MERGE] #2274
Conversation
bajtos
requested review from
raymondfeng,
hacksparrow,
jannyHou,
b-admike,
dhmlau and
nabdelgadir
|
@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 |
|
@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? |
Interesting idea, I haven't considered it before. When using an existing LB3 app in whole, then we need to bring in With my proposed "compat" package, upgrading users are moving completely away from the old npm modules and use LB4 counterparts instead.
Hey, nice to see you again :)
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?
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. |
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, 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. 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. |
|
I created a new spike issue to look into mounting LB3 app in whole: #2301 |
|
Hi 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? |
|
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 :) |
|
@gtamas thank you for the feedback, I appreciate it a lot! ❤️
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
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.
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.
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
Yes 🎉
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. |
There was a problem hiding this 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', () => { |
There was a problem hiding this comment.
provides Model.app.models.Model API here?
| export function setupPersistedModelClass( | ||
| registry: Lb3Registry, | ||
| ): typeof PersistedModel { | ||
| const ModelCtor = registry.getModel('Model'); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
|
|
||
| Remove `_meta` section, LoopBack 4 does not support this config option. | ||
|
|
||
| 4. Modify your Application class and apply `CompatMixin`. |
There was a problem hiding this comment.
nitpick: might want to mention filename here
examples/lb3models/README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| 6. Register your legacy datasources in Application's constructor. |
There was a problem hiding this comment.
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', | |||
| ## 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 |
There was a problem hiding this comment.
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?
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.
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.
This could possibly be a spike story to investigate those two challenges.
I don't have better suggestions at the moment. |
|
@b-admike thank you for a thoughtful review & valuable feedback! |
|
I have discussed the spike with @raymondfeng & @dhmlau and we agree to stop pursuing this direction. While it does provide relatively easy migration path for LB3 users, there are too many downsides for us. Most importantly, we are re-introducing LB3 codebase that we wanted to get rid of, plus we are delaying the time when LB3 users will finally move to the new LB4 design. As a short-term solution, we will allow LB3 users to mount their entire LB3 app in an LB4 project - see #2301, #2318 and the list of follow-up tasks posted in #2318 (comment):
For longer-term, we will look into ways how to simplify migration from LB3 models to LB4. I'll create follow-up spike stories for that soon. |
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.
WE DECIDED TO ABANDON THIS APPROACH IN FAVOR OF #2479. WE ARE ENCOURAGING OUR COMMUNITY TO PICK UP THE CODE PRESENTED IN THIS PULL REQUEST, CREATE A NEW GITHUB REPOSITORY AND CONTINUE THE DEVELOPMENT OF THE COMPATIBILITY LAYER THERE.
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:
supertestshould be trivial to migrate. Integration/unit tests calling JavaScript API directly may require little bit more work, but hopefully few helpers to convert fromapp.{foo}toapp.v3compat.{foo}will do most of the job.How to review this work
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.
In the next step, read the architecture overview to better understand how LoopBack3 parts fit together and how they map to LoopBack 4.
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.
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.
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: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/v3compatat the moment), the example app (@loopback/example-lb3models), but also theApplicationproperty exposing LB3-like app (app.v3compatright now), and so on.TypeScript does not support static index signature, which make it difficult to consume custom model methods. See Allow defining static index signature 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
Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated