Skip to content
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

Fix colocated source maps #763

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wagenet
Copy link

@wagenet wagenet commented Jan 10, 2023

Resolves #558.

@dagroe
Copy link

dagroe commented Jan 24, 2023

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
Copy link
Author

wagenet commented Jan 25, 2023

@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
Copy link

dagroe commented Jan 25, 2023

@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
Copy link
Author

wagenet commented Jan 26, 2023

@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
Copy link

dagroe commented Jan 26, 2023

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

@wagenet wagenet force-pushed the fix-colocated-template-sourcemaps branch from f59355d to 3407a9b Compare January 26, 2023 21:24
@bartocc
Copy link

bartocc commented Jan 27, 2023

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

@bartocc
Copy link

bartocc commented Jan 27, 2023

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
Copy link

dagroe commented Jan 27, 2023

@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
Copy link

bartocc commented Jan 27, 2023

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 dagroe/test-ember-sourcemaps#2 to bundle the launch config you used in your previous post to help others test it

@dagroe
Copy link

dagroe commented Jan 27, 2023

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.

@wagenet wagenet marked this pull request as ready for review January 27, 2023 19:01
@dagroe
Copy link

dagroe commented Mar 22, 2023

@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
Copy link
Author

wagenet commented Mar 22, 2023

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
Copy link

dagroe commented Mar 30, 2023

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
Copy link

dagroe commented Mar 30, 2023

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

@wagenet wagenet force-pushed the fix-colocated-template-sourcemaps branch from 4fc947c to 1ec1645 Compare March 30, 2023 21:06
@dagroe
Copy link

dagroe commented Jul 10, 2023

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

@locks locks self-assigned this Jul 14, 2023
@wagenet wagenet force-pushed the fix-colocated-template-sourcemaps branch 2 times, most recently from daec2a9 to 253bd70 Compare November 15, 2023 22:30
@wagenet
Copy link
Author

wagenet commented Nov 15, 2023

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.

pnpm-lock.yaml Outdated

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming didn't mean to introduce this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops!

@dmyoung9
Copy link

dmyoung9 commented Sep 15, 2024

@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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants