Earlier <LazyMount> migrations
#892
Conversation
| // 2. When that fails, try to access `requirejs` directly. | ||
| // | ||
| try { | ||
| module = await import(`${name}/engine`); |
There was a problem hiding this comment.
this sort of import is 'illegal' under embroider as there is no way to know what it could refer to
There was a problem hiding this comment.
to me, this is kinda impossible to do reliably both for classic Ember CLI build and Enbroider without knowing how Embroider would handle lazy-loaded non-routable engine.
on the other hand, we could have "classic" only implementation like we have for lazy-loaded routable engine being provided by the ember-engines package itself (which would not "just work" under Embroider).
Then, in Embroider we would have [compat-adapter](https://github.com/embroider-build/embroider/tree/main/packages/compat/src/compat-adapters) that override that behavior, once Embroider can handle lazy-loaded non-routable engine.
As I understand after discussion with @void-mAlex, second part of the above currently impossible because Embroider not able to handle lazy-loaded non-routable engine.
And here we get to the gray area, because there is no clear understanding how lazy-loaded non-routable engine should be handled by Embroider, except understanding that Engines in future should be v2 apps per emberjs/rfcs#977 (which is early draft as of writing this).
In that future, v2 apps could be built, deployed and connected together more freely as each app could be an engine or a host app but that's kinda more like an implementation detail.
Unfortunately, this all of the above does not give any answers, only asks more questions. But idea is to outline an understanding of current state of things.
And knowing above, I think it's still valuable to ship this feature knowing it's limited to classic Ember CLI build system because at this point in time we can't do other way.
In parallel, I don't mind to help push emberjs/rfcs#977 if I could as it would benefit both Ember Engines project and wider ecosystem (and I assume would fit Polar goals too).
There was a problem hiding this comment.
I dunno if that was illegal, but nevertheless aware this is "grey area" here. However, as long as it allows to have engines being loaded under embroider this is a win (and then the crime doesn't really matter that much).
I also expect, that once v2 app RFC is more clear, that engines will be rewritten, ie. drop broccoli for unplugin etc. which then would also solve the crime that was lasting long enough to enable people to update to embroider.
So, as long as the consumer API is "stable", the implementation behind the curtains can change and update along the versions.
There was a problem hiding this comment.
The situation to avoid is to wait for app v2 RFC to be finished and then make engines embroider compatible... that'll take too long.
There was a problem hiding this comment.
I also expect, that once v2 app RFC is more clear, that engines will be rewritten, ie. drop broccoli for unplugin etc. which then would also solve the crime that was lasting long enough to enable people to update to embroider.
people can upgrade to embroider today with no issues, so I'd like to clear up that as it feels like that was a misunderstanding somewhere. I'm on latest stable embroider with all static flags on and using lazy routable engines.
this feature would not work though in any scenario where embroider would bring you benefits though and would actually block people from adopting if they tried to use it.
The situation to avoid is to wait for app v2 RFC to be finished and then make engines embroider compatible... that'll take too long.
v2 app rfc needs to progress further than it is now and then there will be no more need for engines package at all, no need for an unplugin or anything of the sorts
the stable api that is needed for engines is how it deals with service isolation which is its own rfc that blocks the v2 app one and how it integrates with routing with is partially blocking the v2 app rfc
There was a problem hiding this comment.
The situation to avoid is to wait for app v2 RFC to be finished and then make engines embroider compatible... that'll take too long.
I couldn't agree more here @void-mAlex @SergeAstapov the Ember Engines RFC build in 2014 will complete 10 years and never were finished. I think is a tragedy to wait for something we're not really sure. At this point we need to complete the original RFC and wait for the new Engines that will come up with this new RFC. I love the way @void-mAlex is planning the new engines and I'll support it, but this time we need to be faithful to what we have today and do our best to make the current engines compatible with Embroider and finish the original RFC.
Please let's avoid overengineering here and make it simple to pay the whole tech debt that made the whole community frustrated and struggling with engines 🙏
The #669 is a good example of a PR opened since 2019 and now @void-mAlex wants to merge into ember-engines my addon by #855, which I greatly appreciate. So that's another proof of concept that we need to finish the original RFC without friction. We need PRs that implement natively here the lazy-routless engines and the Engines Router Service.
Additional Links:
There was a problem hiding this comment.
I agree. This ain't be affecting public APIs (which is lazy mount) but is replaceable under the hood as embroider progresses and can be exchanged behind the curtains at a later point.
The more urgent question is will lazy mount be a helper or component, as this is part of the public API. Once this is settled, I think all the bits and pieces are lying around. Copy from my PR what you need, close it and deliver the finishing blow over at #887 👍
There was a problem hiding this comment.
the only way the public api doesn't change is if it's the one used by the v2 app rfc (tbd at the point in time of this comment)
with that said, one thing to consider is for it to actually be a modifier as you'd technically need access to a dom node to render the engine in (which you would get with a component as well but not quite the same) between component and helper I'd got with component as we can do lazy loading today with that
I do want to stress that your public api will change if you want to support this feature under embroider/vite or whatever comes next
|
@gossi could you please add tests to it including embroider scenario? |
embroider scenario will be added as part of the ember v5 work |
|
The idea of this PR was to push my earlier investigations, as such the PR is not targeted at the That is, take from my PR what you need: Merge it into or copy the relevant lines and close it afterwards. I expect final implementation and testing to happen at #887 What I could offer though: I do have the component and the service available as (g)ts 😬 |
|
@gossi curious which ember source you're on and which embroider flags you have one + embroider version the import statement does not work now and will never work in the future as written no matter the build tool used |
|
Ah, I did use the addon v2 blueprint and used the test-app within there (with In // const { maybeEmbroider } = require('@embroider/test-setup');
// return maybeEmbroider(app);
return app.toTree();But I see, it only has |
Heyo 👋
This was my original
<LazyMount>migration, to support embroider and classic builds. I modernized the component with a glimmer component (such no positional params) but named blocks and updated the loading of the engine with something that works under embroider as well as requirejs land.I think, the loading part is the core of this PR.
While doing so and looking at the "modernizations" to
<LazyMount>that sorta diverge from{{mount}}- I think it might be better to revert this<LazyMount>component back into a{{lazy-mount}}helper.Either way should be fine, but keeping the helper as an ember component is probably the worst decision to be made here.