ember-cli-htmlbars icon indicating copy to clipboard operation
ember-cli-htmlbars copied to clipboard

Fix colocated source maps

Open wagenet opened this issue 2 years ago • 18 comments

Resolves #558.

wagenet avatar Jan 10 '23 04:01 wagenet

Thanks for you MR! I'd really like to see this issue fixed for easier debugging.

I tried it on our app, but unfortunately don't get it to work properly.

Without this fix we get sourcemaps for our application-name.js that point to our components' .ts files, although with the offset in line numbers.

With this fix we get sourcemaps for our application-name.js that point to the transpiled .js files. The mapping to .ts files is missing.

When looking in to it, it seems your changes correctly apply the template prefix and add the sourcemap to our .ts files. But then those sourcemaps seem to be incompatible with the babel transpilation.

dagroe avatar Jan 24 '23 18:01 dagroe

@dagroe Yeah, this is the problem I have. I expected this change to fix stuff, but it doesn't seem to have. I have to dig in more. These source maps should be ingestible by other tools, so I'm not sure what's going on there. Maybe we have to change a downstream setting.

wagenet avatar Jan 25 '23 01:01 wagenet

@wagenet Glad I am not the only one with this problem 😉

I did a bit more digging.

We do not get sourcemaps since the generated sourcemaps fail validation. When sourcemaps are built by broccoli-concat it uses fast-sourcemap-concat which in turn runs validation via sourcemap-validator. Here validation simply fails because our sourcemaps are missing the correct column positions.

Luckily we can fix that by passing hires: true to jsContentsMagic.generateMap.

Second issue is that the sourcemaps do not have the correct paths to sources. E.g. a component in components/list/item.ts will only have item.ts in sources. What works for me was the following:

Store the correct extension:

let ext;
if (this.inputHasFile(basePath + '.js')) {
  backingClassPath += '.js';
  ext = '.js';
  hasBackingClass = true;
} else if (this.inputHasFile(basePath + '.ts')) {
  backingClassPath += '.ts';
  ext = '.ts';
  hasBackingClass = true;
} else if (this.inputHasFile(basePath + '.coffee')) {
  backingClassPath += '.coffee';
  ext = '.coffee';
  hasBackingClass = true;
} else {
  backingClassPath += '.js';
  ext = '.js';
  hasBackingClass = false;
}

then use it so set the correct file and source:

let file = filePathParts.name + ext;
let jsContentsMap = jsContentsMagic.generateMap({
  source: backingClassPath,
  file,
  includeContent: true,
  hires: true
});

This seem to generate correct sourcemaps.

dagroe avatar Jan 25 '23 13:01 dagroe

@dagroe Great! I'm so glad you were able to look into this. Would you prefer to make your own PR or should I make the fixes in my PR when I'm able?

wagenet avatar Jan 26 '23 15:01 wagenet

@wagenet Thanks for merging my changes :)

Maybe we can get someone else to test this as well. I know it's been some time but maybe @beddueimpossibile or @bartocc are still interested?

I'll do some more testing. So far source maps look fine when debugging in chrome or firefox but I haven't been able to make it work correctly in pycharm yet.

dagroe avatar Jan 26 '23 21:01 dagroe

Thx for flagging me! I am still very interested and I would love to see this fixed! Gonna try this PR right now!

bartocc avatar Jan 27 '23 08:01 bartocc

I've done some testing on our app and with VSCode.

I've changed package.json with

- "ember-cli-htmlbars": "^6.1.0",
+ "ember-cli-htmlbars": "wagenet/ember-cli-htmlbars#fix-colocated-template-sourcemaps",

setup VSCode launch setting with:

"sourceMapPathOverrides": {
  "my-app/*": "${workspaceFolder}/app/*"
}

Unfortunately, I do not see any improvements. Here are my results:

classic structure + JS component class: breakpoint binds

app/components/foo-bar.js app/templates/components/foo-bar.hbs

classic structure + TS component class: breakpoint binds

app/components/foo-bar.ts app/templates/components/foo-bar.hbs

flat structure (ie co-located template) + JS component class: breakpoint DOES NOT BIND

app/components/foo-bar.js app/components/foo-bar.hbs

flat structure (ie co-located template) + TS component class: breakpoint DOES NOT BIND

app/components/foo-bar.ts app/components/foo-bar.hbs

bartocc avatar Jan 27 '23 09:01 bartocc

@bartocc I tested it using a new dummy project, which I put here: https://github.com/dagroe/test-ember-sourcemaps

The breakpoint in components/test/foo-bar.ts was hit using vscode with this launch.json:

{
    // Use IntelliSense to learn about possible attributes.
    // Hover to view descriptions of existing attributes.
    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
    "version": "0.2.0",
    "configurations": [
        {
            "type": "chrome",
            "request": "launch",
            "name": "Launch Chrome against localhost",
            "url": "http://localhost:4300",
            "webRoot": "${workspaceFolder}",
            "sourceMapPathOverrides": {
                "html-bars-test/*": "${workspaceRoot}/app/*"
            },
        }
    ]
}

Still trying to figure it out for pycharm, but the sourceMapPathOverrides seems to be the critical part in vscode.

dagroe avatar Jan 27 '23 11:01 dagroe

Thx for setting up this demo project @dagroe. I need to check our own project to see why the breakpoints do not bind for co-located components…

But if I am correct, everything works fine when using wagenet/ember-cli-htmlbars#fix-colocated-template-sourcemaps, doesn't it?

PS: In the mean time, I've prepared a small PR https://github.com/dagroe/test-ember-sourcemaps/pull/2 to bundle the launch config you used in your previous post to help others test it

bartocc avatar Jan 27 '23 11:01 bartocc

Thanks for your PR :)

Works fine for me when using wagenet/ember-cli-htmlbars#fix-colocated-template-sourcemaps. Source maps look correct in browser debug (firefox and chrome) and work as expected in VSCode. Also making changes, reloading etc. seems to work fine.

dagroe avatar Jan 27 '23 12:01 dagroe

@chriskrycho @rwjblue @dfreeman sorry for the ping but I am not sure who to ask for a review of this. Could anyone of you do it or assign someone? Thanks!

dagroe avatar Mar 22 '23 07:03 dagroe

I just tried this is another app of my own and it appeared to drastically slow down the build. I'm not sure if the speed issue is actually related or if it's just a red herring. I'd like to try to get that figured out soon.

wagenet avatar Mar 22 '23 15:03 wagenet

That would be a big drawback 😞 , but it seems to make sense that generating sourcemaps with hires: true for all colocated components will slow down the build.

I was thinking that there might be a quicker way to generate the sourcemaps since source and generated files should be identical besides the two added lines for the template. I need to find some time to look into whether it makes sense to build the sourcemaps ourselves.

dagroe avatar Mar 30 '23 07:03 dagroe

Seems generating the source map is straight forward. I added it here: https://github.com/dagroe/ember-cli-htmlbars/tree/try-custom-sourcemap-generation @wagenet can you give this a try to see if it still works and improves build times in your project? 😄

dagroe avatar Mar 30 '23 13:03 dagroe

@wagenet Seems source maps are fixed in 6.2.0, at least for me they are working again without this PR.

dagroe avatar Jul 10 '23 08:07 dagroe

I took another pass at this and figured out a couple things. First, I fixed the paths so that source maps should fully work. (Note that because of unrelated issues in the build process, you may need to set them as "inline" in your babel config.)

As far as concerns about the build being slowed down, this seems to be due to a bad interaction with ember-template-imports v3 which parses all .js and .ts files. Somehow adding the source maps can end up being very bad for this.

wagenet avatar Nov 15 '23 22:11 wagenet

@wagenet Seems we are not able to install your fork directly via npm, due to this line.

I was able to get it to work by cloning locally, removing that line, and installing it from my local filesystem, but this is not ideal.

Would it be possible to remedy this at the source?

EDIT: I can install it using pnpm, but this seems to cause issues with my other dependencies.

dmyoung9 avatar Sep 15 '24 17:09 dmyoung9

I took another pass at this and figured out a couple things. First, I fixed the paths so that source maps should fully work. (Note that because of unrelated issues in the build process, you may need to set them as "inline" in your babel config.)

As far as concerns about the build being slowed down, this seems to be due to a bad interaction with ember-template-imports v3 which parses all .js and .ts files. Somehow adding the source maps can end up being very bad for this.

Unfortunately, using sourceMaps: 'inline' breaks debugging in Chrome DevTools... If I use just your fork, with no other modifications, debugging works in Chrome DevTools and code coverage is accurate, but I still haven't been able to get VS Code breakpoints to bind properly.

dmyoung9 avatar Sep 15 '24 17:09 dmyoung9