ember-cli / ember-cli-htmlbars

MIT License
77 stars 66 forks source link

Fix colocated source maps #763

Open wagenet opened 1 year ago

wagenet commented 1 year ago

Resolves #558.

dagroe commented 1 year ago

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.

wagenet commented 1 year ago

@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.

dagroe commented 1 year ago

@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.

wagenet commented 1 year ago

@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?

dagroe commented 1 year ago

@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.

bartocc commented 1 year ago

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

bartocc commented 1 year ago

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

dagroe commented 1 year ago

@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.

bartocc commented 1 year ago

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

dagroe commented 1 year ago

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 commented 1 year ago

@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!

wagenet commented 1 year ago

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.

dagroe commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

wagenet commented 10 months ago

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.