-
-
Notifications
You must be signed in to change notification settings - Fork 66
Co-location compiler may not work with pods or TypeScript? #308
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
Comments
One of the nice things about co-location is that we see a future without pods. For folks already using pods, migrating to co-location components would feel like home, but if Ember requires to choose one or the other, pods would still be chosen instead of co-location. I believe that migrating to co-location when using pods is a small lift, but it would be a very hard sell if Ember would be required to move routes, controller, templates from pods to classic just to get co-location. |
The issue here is that its just not clear what to specifically do. I think that we should not break if pods are used, but I do not think that pods themselves should be allowed to use colocation. @josemarluedke - Can you share some examples from your app of how you are using pods? It would be really useful to have some real-world examples to use for test cases for the broccoli-plugin (I'm mostly looking for file layout, not the contents of your components and whatnot). |
Thank you! Is there a specific issue you hit when interoping today? As in, if you move a single component to be colocated what happens? Does it work, does it error, etc? |
@rwjblue As I mentioned in emberjs/ember.js#18350 (comment), by moving one component to co-location, it won't render anymore. |
Hmm. Ok. I’ll spend some time trying to reproduce, but if others have a demo repo that shows the issue that would be helpful. There may be one in emberjs/ember.js#18350, will double check. |
ember-learn/super-rentals-tutorial#72 seems to have confirmed that there are no issues with pods. @josemarluedke did some more digging, and the problem may have to do with TypeScript instead. |
If you are running into issues, a good thing to do may be to fork https://github.com/chancancode/super-rentals-pods-ish/ and try to reproduce the problem. |
Ya, we really need a reproduction in order to dig in. There are many variations of "pods" and folks tend to all be talking about different things, making it very hard to actually dig into... |
It seems this issue is not related to pods after all, just TypeScript. My original urge to comment here was because I tested in an app using pods and typescript (which is were I saw errors). Testing out with super-rentals-pods-ish showed that there was no issues in pods with co-location. Both seem to work well together. Thanks @chancancode for putting that together, it really helped. The issue I have seem to be present only when a decorator is applied to a co-located component when the project has TypeScript, independent if the component is JS or TS. I have a fork of super-rentals-pods-ish showing the problem. The component "other" won't render anything. Below are 2 screenshots of the compiled component JS file, one not using TypeScript and the other using it. When TypeScript was not installed in the projectCorrespondent commit: https://github.com/josemarluedke/super-rentals-pods-ish/commit/a5536151f9b9c37fd56f5bcb3e36dc103a45e190 When TypeScript was installed in the projectCorrespondent commit: https://github.com/josemarluedke/super-rentals-pods-ish/commit/54464c0d6de9b729ed96dc584dcba966c1daf16c |
@josemarluedke Could be related to this: typed-ember/ember-cli-typescript#724 |
I suspect that #333 will fix this, but if not it at least should make it easier to dig in to what is going on. |
OK, #333 is released in 4.0.6 please test and confirm it that fixes these issues... |
I just tested this out and it did not solve the issue. See reproduction repo here: |
Thanks to @dfreeman, @HenryVonfire, and @josemarluedke, this should now be fixed and released in v4.0.7. Please test / confirm, then I'll close this issue... |
I just confirmed that v4.0.7 did fix the issues I have been seeing. Thanks, @dfreeman, @HenryVonfire, @rwjblue. |
Do we intend to make it work? If not it should error.
The text was updated successfully, but these errors were encountered: