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

Allow for URL as sourceRoot #1427

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

FrankEssenberger
Copy link

Relates to #1424.

@connor4312 I created a small PR. I was a bit lost in the code base. The whole SourceContainer has so much state that I could not get a unit test running. I then just added a test and checked that the fetchHttp is called and the sources are really loaded from the local host.

The behaviour in the getComputedSourceRoot is now a bit inconsistent for the file:// the protocol is removed and later added, which should of course not happen if it is a http:// source. A proper solution would perhaps to have an object not only a string for the URL. Then the object could hold the information I am file, data, http, ftp or whatever.

I did not want to change much so I added just minimal checks.

@FrankEssenberger
Copy link
Author

@microsoft-github-policy-service agree company="SAP"

@FrankEssenberger
Copy link
Author

FrankEssenberger commented Oct 17, 2022

Ok, I found out that the VSC experience is much better compared to webstorm my default IDE. I wanted to test the flow E2E and replaced the source root also with https://raw.githubusercontent.com/SAP/cloud-sdk-js/main/packages/util/src and I saw a new assertion with the source from our repo. So far so good.

However, when I run the launch config extension and use the sample repo it does not work and I get an error with a better URL (no file system prefix)

Could not load source '/https:/raw.githubusercontent.com/SAP/cloud-sdk-js/main/packages/util/src/string.ts': Unable to retrieve source content.

but I have no Idea why it is not working. When I debug in the repo (golden tests) the URL is correct and source code can be fetched. Is there some setup needed? Is there a way to debug the code used by the debugger to see what the error on the fetchUrl is?

@FrankEssenberger
Copy link
Author

FrankEssenberger commented Oct 21, 2022

I spend s bit of time to find out why it is working in the repo but not with the separate repo. I debugged the debugAdapter.ts because some errors sounded like they could lead to the error I see in the manual test using the Extension launch and the separate repo. However, using the same URL as source route the content is just downloaded and I see it:

image

So there must be something different with the two cases but I have no idea how to debug the Extension launch. Perhaps you can give me some hint? @connor4312. I have the nightly debug extension installed and in the VSC window I see the Extension Host as a name:

image

Since this morning I also get an other error message (merged main) which seems like the new code is not considered at all. Hints to debug in the extension mode would be appreciated. Perhaps my understanding is also wrong - the CONTRIBUTION.md was rather short.

Best
Frank

@connor4312
Copy link
Member

Looking at this this week. I think there should be a slightly wider change here to prefer URIs to absolute paths. Unfortunately DAP is only path-based and doesn't know about URIs, but for example urlToAbsolutePath should be renamed if it doesn't always return absolute paths, and instead we should always return URIs (instead of requiring the caller to check what it received).

I've started making changes based on this PR and hope to have this merged on or by Friday.

@FrankEssenberger
Copy link
Author

@connor4312 cool that you take this over. I was afraid about bigger refactoring since it is not my code base :-). I was also lacking the deeper understanding and some tests seems flaky on my machine or in the pipeline. Anyway would be great if this works so that we can improve the debug experience without increasing the size of the shipped artifact.

@FrankEssenberger
Copy link
Author

Perhaps we can mention your PR once it is there for reference and close this one here.

@uplight-dev
Copy link

Is there anyone reviewing this?

Copy link

@New-templates1990 New-templates1990 left a comment

Choose a reason for hiding this comment

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

pull request gh pr checkout 1427

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.

4 participants