-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Place colocated templates after the default export #558
base: master
Are you sure you want to change the base?
Place colocated templates after the default export #558
Conversation
I certainly don't mind the ordering change, but I wonder if we should be doing something more intelligent WRT sourcemaps all together. Perhaps even including an inline sourcemap for the modifcation that we are doing here. 🤔 |
Yes I agree with you, I improvised actually. Unfortunately I am not mastering those tools so I tried to solve it the way I know. |
An other possible solution, as you pointed out, is to generate the map file relative to the original js source file instead of the processed one. Unfortunately I could not figure out how. I tried to look for it yesterday. For a "layman" like I am it's all but trivial to move inside the ember projects and I could not find out the right spot and/or the right tool. So I decided to implement this fix which is not that invasive and does the trick, with some negligible(?) overhead. I would be glad to help, just need someone to point me at the right target since unfortunately my job is pressing me in those days and I don't have enough time to study the projects. For sure I am available for testing other solutions on field since I am working on ember projects on daily basis. |
Unfortunately, this PR is not working in our project. We have in-repo addons that use the @rwjblue could you give me some indications about how to fix the sourcemaps instead of doing what is done in this PR 🙏? |
@bartocc - We have to do something smarter than simply inserting the string contents. My first guess would be something akin to magic-string. |
@beddueimpossibile when you say "Only debugging from Chrome is possible." What do you mean by this? Why is Chrome different than VSCode in this regard? My best guess is: when viewing in Chrome, the content of the original file is constructed using sourcemaps, and while it does include the embedded colocated template, the source mapping to [original file + injected colocated component] is actually correct, so breakpoints function properly within Chrome. But within VSCode: VSCode doesn't "reconstruct" the source file with sourcemaps; it just uses the source file and expects the source maps to map to that exact source file, but due to the injected co-located template, it is NOT an exact match, hence VSCode debugging doesn't work properly. Is that a correct understanding? |
I don't know what is the percentage of Ember.js developers using colocated templates and VSCode 🤔, but I am surprised this issue has not tracked more attention. Being able to use breakpoints in colocated source files within VSCode would be, IMHO, a big DX improvement. |
It was a couple of years ago so I don't have a crystal clear picture in my mind about the situation, but I am quite sure that with Chrome I could ignore the map files and just step-into the generated JS code. It is also interesting that this PR started pinging me back right in these days. I am going to work with Ember.js again on a production project after a long pause, so I might be able to refresh my mind about this and maybe come back with insights! |
I am not 100% certain but I suspect this issue will not be fixed for co-located template components. The way it's implemented is very complicated, ultimately due to there being two files that are essentially mixed into one in the build tooling, and I doubt there is community or core team energy to dig this up and fix it. The future probably lies in using Single File Components, facilitated by First-Class Component Templates. SFCs, e.g. I know the above might come off as a bold or annoying statement, but if it's generally correct, it'd be nice to get a 👍 from an official maintainer or other people in the know. |
I was more than reluctant to move away from our actual use of co-located template components and refactor to use the But I think you are right @machty
and we will probably migrate our codebase in that direction to stay up-to-date with the framework's best practices. And if this fixes this VSCode breakpoint issue, then it'll be another strong incentive! |
Chrome uses the sourcemap as code, so it just has the 2 extra lines. IDEs use the actual code. |
I noticed that the source maps for component js files have a line number offset and so breakpoints and watches in VSCode doesn’t work properly. Only debugging from Chrome is possible.
After short investigation I found out that it is due to the template colocation feature. It prepends the template to the component class and so the resulting line numbers doesn't match the ones of the original js file.
I thought about appending the template to the component class instead of prepending it, in order to have the colocation feature not braking map files.
It seems working, the tests are updated and passing. I am already testing it on my local machine on production projects and I can easily switch back to the current version, so in the next days I will have detailed feedback. I decided to send a pull request in advance to have also your idea about this.