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

Bring back source map support #2166

Closed
alfonsogarciacaro opened this issue Sep 15, 2020 · 35 comments · Fixed by #2337
Closed

Bring back source map support #2166

alfonsogarciacaro opened this issue Sep 15, 2020 · 35 comments · Fixed by #2337

Comments

@alfonsogarciacaro
Copy link
Member

alfonsogarciacaro commented Sep 15, 2020

It's likely that Fable 3 will ship initially without F# source map support (we're trying to compensate that with more readable JS output) but the infrastructure to generate them is still in place. As Fable 3 is a "pure" dotnet app, we need a dotnet library to generate the source maps but we couldn't find any that meets our needs. The easiest way is probably to translate the sourcemap generator of Mozilla's JS library to dotnet (either F# or C#). It'd be great if the community could help with that.

@alfonsogarciacaro alfonsogarciacaro added this to To Do in Nagareyama via automation Sep 15, 2020
@Zaid-Ajaj
Copy link
Member

Will Fable 3 no longer be distributed via the fable-compiler package (used alongside fable-loader)? 😱 I understand that the pure Fable app has a better user experience than fable-splitter to make node.js apps but removing the npm distribution would be huge change everywhere and honestly a bit of a pain in the back to work with: templates, libraries and projects. It is just sooo much easier to say: npm upgrade fable-compiler and it will be ready to go (hopefully without breaking changes)

@alfonsogarciacaro
Copy link
Member Author

Since they fixed the version-pinning of local tools, distributing Fable 3 as a dotnet tool is likely the way to go, as all the other F# tools are doing (Paket, Fake, Fantomas, Femto, Snowflaqe). Keeping fable-compiler as a parallel distribution will probably be more confusing to the users than having a clear cut.

I would prefer if people got used to call Fable as a dotnet tool 😸 But... I do understand that updating all tutorials, projects, templates is a pain (as it's been in the past). So we could consider to release a new fable-loader major version that just invokes the Fable dotnet tool. The steps to upgrade (when stable versions are release) would then just be:

dotnet tool install fable
npm upgrade fable-loader

Probably also adding *.fs.js to .gitignore. fable-compiler can be uninstalled or not, it will just not be invoked. How would that look? And, would someone volunteer to maintain the new fable-loader? 😉

@Zaid-Ajaj
Copy link
Member

Keeping fable-compiler as a parallel distribution will probably be more confusing to the users than having a clear cut.

Keeping backward-compatibility with the least changes required is really important and would make many people happy. Right now, it sounds more like a downgrade because of the level of indirection introduced (fable has be called first, then webpack) and webpack will expect files to exist only after Fable compiled them whereas right now it is totally invisible and looks a lot like how TypeScript is integrated with existing projects.

I do like the CLI tool to simplify building and running node.js applications instead of using fable-splitter

Since they fixed the version-pinning of local tools, distributing Fable 3 as a dotnet tool is likely the way to go

Maybe it is an idea to make a poll (on twitter for example) to ask existing users what they would prefer?

@alfonsogarciacaro
Copy link
Member Author

Moving the discussion to #2195 as this issue is about the source maps and it can be confusing for contributors.

@piaste
Copy link
Contributor

piaste commented Oct 27, 2020

Without source maps there will be no way to integrate with step-by-step debugging in VSCode via launch.json, is that correct?

I'm guessing most front-end users will prefer using a time-travelling debugger anyway.

@alfonsogarciacaro
Copy link
Member Author

You can still use the VSCode debugger but breakpoints can only be hit in the generated JS files. I did use the source maps frequently both in VSCode and Chrome (although sometimes name mangling made it difficult to identify values, something we're trying to improve in Nagareyama), but I don't know if many other users did.

@jpierson
Copy link

I haven't started any code yet on this but I'm keeping my eye on this. My first inclination is the do a direct port of mozilla/source-map assuming that this is precisely what is needed but then I'm wondering if we'd be better to go with C# or F# for the port, I'd prefer to write it in F# myself but there is some benefits in choosing C#. Either way porting the library would provide a native .NET implementation for working with source-maps in general which may be a useful thing for the .NET ecosystem. For now I've forked the project with some intent of giving this option a shot in my limited spare time.

Another option perhaps that just occurred to me would be to use WebAssembly support for mozilla/source-map library to compile to WebAssembly and then embed the WASM into a .NET library using Wasmtime. I'm not that familiar with this option but if it works and performs reasonably it may be an easier way to keep the implementation in sync with the original source-maps library in Javascript.

@jwosty
Copy link
Contributor

jwosty commented Dec 20, 2020

Another option perhaps that just occurred to me would be to use WebAssembly support for mozilla/source-map library to compile to WebAssembly and then embed the WASM into a .NET library using Wasmtime. I'm not that familiar with this option but if it works and performs reasonably it may be an easier way to keep the implementation in sync with the original source-maps library in Javascript.

Almost sounds like we need a Java-script to F# transpiler... 🤷

In all seriousness, an F# source map library would be a good idea.

@delneg
Copy link

delneg commented Dec 23, 2020

https://github.com/delneg/source-map-sharp
Started some work translating https://github.com/mozilla/source-map/blob/master/lib/source-map-generator.js
there, not the best code at all but I tried to do a direct translation

@alfonsogarciacaro
Copy link
Member Author

This is great @delneg, thanks a lot! I think a direct translation works best even if it's not F# idiomatic in case we need to sync updates of the original library later 👍

@delneg
Copy link

delneg commented Dec 24, 2020

I've done some work (here https://github.com/delneg/source-map-sharp), but I might need help with the "util.js" functions such as util.join,util.relative which are used in source-map-generator.js

I almost sure we won't need util.getArg because of F# type safety, and i'm almost sure we won't need util.toSetString because it's a hack to avoid 'proto'-related bugs.

Please also tell me if we'll be using that as a CLI or ... ?

@alfonsogarciacaro
Copy link
Member Author

Thanks @delneg! I will have a look over the weekend and I'll try to PR those functions 👍 Yes, the library will be used from Fable.Cli. If you publish it as an independent library we can just reference your Nuget package.

@delneg
Copy link

delneg commented Dec 27, 2020

I've done most of SourceMapNode, SourceMapGenerator, and created a README.md to showcase the current progress.
Also, you can find what kind of help is needed atm.

According to the docs here https://github.com/mozilla/source-map#generating-a-source-map , what we have now is sufficient to generate source maps ... (altough, i'm not entirely sure how atm)

@alfonsogarciacaro
Copy link
Member Author

This is fantastic @delneg! I gave it a quick try and seems to be working 👍 I will now try to enable debugging.

@delneg
Copy link

delneg commented Dec 29, 2020

That's nice, can you propose the next steps ?
I.e. publishing nuget or writing tests or something else (maybe something from the README in the repo)
(P.S. although i've never done nuget publishing, and should probably be done using fable account)
P.P.S no wonder it worked right away, i've got quite used to that while using F#

@alfonsogarciacaro
Copy link
Member Author

Next steps should be testing that sourcemaps actually work for debugging and do the extra work in Fable (adding --sourceMap option, saving it to a file`. I can do this on my side. I will also send you a PR to polish a bit the API (like using optional arguments instead of explicit options).

Right now we don't have a Fable Nuget account, we are publishing the packages with our personal account and usually 2-3 owners just in case. If you create an account in nuget.org and generate a token, it's easy to automate publishing. I can PR the script for that. You can also add me as collaborator of the package if you want.

@delneg
Copy link

delneg commented Dec 30, 2020

Ok, I'll check out nuget publishing stuff when i'll have time
Also, i've added you as a collaborator to the repo
If i'll be able to, I'll try to polish the API a bit too, and try to add some tests for SourceGenerator

@delneg
Copy link

delneg commented Jan 2, 2021

I've started adding more tests for SourceMapGenerator, they revealed some bugs that were hiding.
Some are now fixed, but before it seems like all of them need to be fixed - otherwise mappings may not work in some cases
So, it's a little bit early to publish nuget atm
If anyone wants to help - just take a look at failing tests (dotnet test)

@delneg
Copy link

delneg commented Jan 5, 2021

https://www.nuget.org/packages/source-map-sharp/
I've published nuget package, tests for SourceMapGenerator related actual mappings generation (SerializeMapping function) are done & the bugs are fixed, so it should work properly.
I'll continue on SourceNode & other stuff, and it would be great if some1 could help out with util.relative/util.join

@alfonsogarciacaro
Copy link
Member Author

This is great @delneg! Thanks a lot for this! I'm slowly getting back to work after the holidays so I'll try to push a Fable 3.1 beta release with source map support using your package when possible 👍

I think Fable itself doesn't need SourceNode but if you prefer it to add it for completeness it may help other consumers of the library. About util.relative/join, I will also try to send a PR to your project, but given that this will run on .NET I guess you should be able to use System.IO.Path.GetRelativePath and System.IO.Path.Combine: https://docs.microsoft.com/en-us/dotnet/api/system.io.path?view=net-5.0

@delneg
Copy link

delneg commented Jan 5, 2021

Unfortunately, GetRelativePath is not avaiable for netstandard2.0 (see https://docs.microsoft.com/en-us/dotnet/api/system.io.path.getrelativepath?view=net-5.0#applies-to)
The solution can be to bump to netstandard2.1 (i don't know if it's a good one though )
Regarding util.join - after looking through all occurences, i believe it's used only in consumer-related cases (which i won't be doing atm), so actually it's not that needed right now.

P.S. Regarding the SourceNode etc. - i think if we're doing a .net port of sourcemap, let's do it properly implementing all the stuff that there is, even if it's not used now it may be needed in a year or two

@jwosty
Copy link
Contributor

jwosty commented Jan 5, 2021

.Net Standard 2.1 would leave Mono-related things in the dust (i.e. Xamarin things). But for the Fable use case that's probably fine as it's a development-only dependency and the framework doesn't really mean anything at runtime anyway.

So if the library is only gonna be used in Fable, 2.1 is fine, but if you want maximum compatibility with other .Net stuff, 2.0 is more ideal for that.

FWIW, someone provided a simple implementation of it on StackOverflow: https://stackoverflow.com/questions/275689/how-to-get-relative-path-from-absolute-path

@delneg
Copy link

delneg commented Jan 5, 2021

.Net Standard 2.1 would leave Mono-related things in the dust (i.e. Xamarin things). But for the Fable use case that's probably fine as it's a development-only dependency and the framework doesn't really mean anything at runtime anyway.

So if the library is only gonna be used in Fable, 2.1 is fine, but if you want maximum compatibility with other .Net stuff, 2.0 is more ideal for that.

FWIW, someone provided a simple implementation of it on StackOverflow: stackoverflow.com/questions/275689/how-to-get-relative-path-from-absolute-path

I think for now i'm gonna bump it to 2.1 then, and leave a note in README for potential netstandard2.0 users.
If anyone will need it to work under 2.0, we'll have a fallback solution from Stackoverflow.

Edit: uploaded 1.0.1 https://www.nuget.org/packages/source-map-sharp/1.0.1 with netstandard2.1

@jwosty
Copy link
Contributor

jwosty commented Jan 5, 2021

Another option would be to conditionally exclude (with #if) the things that rely on GetRelativePath via multitargeting, so that everything else would be available on .Net standard 2.0.

@delneg
Copy link

delneg commented Jan 5, 2021

Another option would be to conditionally exclude (with #if) the things that rely on GetRelativePath via multitargeting, so that everything else would be available on .Net standard 2.0.

Seems like an overcomplication for a simple relative URL's problem (atm only that requires GetRelativePath )

@alfonsogarciacaro
Copy link
Member Author

Fable as a dotnet tool is netcoreapp3.1 so it's ok to target netstandard2.1, just you may need to normalize the path when run in Windows like Path.GetRelativePath(path, pathTo).Replace('\\', '/').

If you want the library to target netstandard2.0 for maximum compatibility as @jwosty points out, we actually have our own implementation. I haven't touched it in a while but it seems to work fine:

/// Creates a relative path from one file or folder to another.
let getRelativeFileOrDirPath fromIsDir fromFullPath toIsDir toFullPath =
// Algorithm adapted from http://stackoverflow.com/a/6244188
let pathDifference (path1: string) (path2: string) =
let mutable c = 0 //index up to which the paths are the same
let mutable d = -1 //index of trailing slash for the portion where the paths are the s
while c < path1.Length && c < path2.Length && path1.[c] = path2.[c] do
if path1.[c] = '/' then d <- c
c <- c + 1
if c = 0
then path2
elif c = path1.Length && c = path2.Length
then String.Empty
else
let mutable builder = ""
while c < path1.Length do
if path1.[c] = '/' then builder <- builder + "../"
c <- c + 1
if builder.Length = 0 && path2.Length - 1 = d
then "./"
else builder + path2.Substring(d + 1)
// Add a dummy file to make it work correctly with dirs
let addDummyFile isDir path =
if isDir
then Combine (path, Naming.dummyFile)
else path
// Normalizing shouldn't be necessary at this stage but just in case
let fromFullPath = normalizePath fromFullPath
let toFullPath = normalizePath toFullPath
if fromFullPath.[0] <> toFullPath.[0] then
// If paths start differently, it means we're on Windows
// and drive letters are different, so just return the toFullPath
toFullPath
else
let fromPath = addDummyFile fromIsDir fromFullPath
let toPath = addDummyFile toIsDir toFullPath
match (pathDifference fromPath toPath).Replace(Naming.dummyFile, "") with
| "" -> "."
// Some folders start with a period, see #1599
| path when isRelativePath path -> path
| path -> "./" + path
let getRelativePath fromFullPath toFullPath =
// This is not 100% reliable, but IO.Directory.Exists doesn't
// work either if the directory doesn't exist (e.g. `outDir`)
let isDir = GetExtension >> String.IsNullOrWhiteSpace
// let isDir = IO.Directory.Exists
getRelativeFileOrDirPath (isDir fromFullPath) fromFullPath (isDir toFullPath) toFullPath

@alfonsogarciacaro
Copy link
Member Author

Also Path.GetRelativePath doesn't add a period always in front of the relative path:

> Path.GetRelativePath("/foo/bar", "/foo/bar/hoho/mir");;
val it : string = "hoho\mir"

The period is probably needed for sourcemaps urls, so you may also need to do something like in the lines 545-548 of the excerpt above when normalizing the path.

@delneg
Copy link

delneg commented Jan 6, 2021

I'll check out the stuff above & adapt tests from JS (there's quite a bunch in test-util.js), probably will use our own implementation.
Also, a couple of questions are left:

  1. Maybe we should migrate the discussion to source-map-sharp repository issues ?
  2. Are we planning to use WASM compilation of some sort for this project (is there a reason to do that, because i don't know the reason of WASM usage in original source-map repo atm) ?
  3. Is there anything else needed for Fable to start using source-map-sharp, if anything at all (including docs, tests, additional API's etc.)

Edit: OK, that was easy, already added the custom Util.getRelativePath, added tests and after slight modifications they went green.
Should we revert to netstandard2.0 then and/or publish 1.0.2 nuget?

@alfonsogarciacaro
Copy link
Member Author

Awesome @delneg! 👏 🚀 👏

  1. Yes it makes sense to move discussion to source-map-sharp repository 👍 Please mention me when you need my input so I get the notification.
  2. Didn't check WASM usage in original source-map, but I assume they use it in environments supporting WASM to speed up numeric calculations. I don't think we need to worry about it in the .NET port.
  3. It should be fine, I just didn't have much time these days, but I'll try to publish a 3.1 beta release soon with source maps 💪 You can go ahead and publish 1.0.2 so we use this in the beta.

@alfonsogarciacaro
Copy link
Member Author

Fable 3.1 beta is published with source map support thanks to @delneg fantastic work! 🎉 https://twitter.com/FableCompiler/status/1347421291502997504

@JamesRandall
Copy link

Fantastic - from a Fable user: many thanks for this @delneg !

Nagareyama automation moved this from To Do to Done Jan 8, 2021
@jwosty
Copy link
Contributor

jwosty commented Jan 8, 2021

Amazing amazing amazing!

@delneg
Copy link

delneg commented Jan 8, 2021

Awesome @delneg! 👏 🚀 👏

1. Yes it makes sense to move discussion to source-map-sharp repository 👍 Please mention me when you need my input so I get the notification.

2. Didn't check WASM usage in original source-map, but I assume they use it in environments supporting WASM to speed up numeric calculations. I don't think we need to worry about it in the .NET port.

3. It should be fine, I just didn't have much time these days, but I'll try to publish a 3.1 beta release soon with source maps 💪 You can go ahead and publish 1.0.2 so we use this in the beta.

Thanks for your support.
I'll try my best to maintain source-map-sharp in the future, and I'm glad it works now.

  1. I'll write in the repo's issues then, and if anyone has any questions etc. - please open an issue in source-map-sharp
  2. Yes, i suppose they did use WASM to improve performance.
    I did try out and get WASM version of source-map-sharp working, however atm the state of WASM compilation in .net seems awful and very hard to use (some work is done in Uno.Platform.Bootstrap project, but looking at it's source code did disappoint me very much)
    As far as source-map-sharp is kept a .NET application, if we'll ever need more perfomance we can always look at Span and other .NET stuff to make it faster
  3. I did publish 1.0.2 and i saw that you've already used it, so that's that.
    I'll try to continue publish Nuget's in the future if we find any bugs etc. (and try not to change the API)

@bzuu-ic
Copy link

bzuu-ic commented Feb 2, 2021

For anyone still not seeing the source maps after applying the instructions from @alfonsogarciacaro, try to clean your output files (including all the .fs.js ones) and run your build again. It took me a while to figure this out as simply rebuilding without cleaning didn't work.

Other than that, thanks to everyone involved for bringing this great feature back 👍

@jpierson
Copy link

jpierson commented Apr 3, 2021

Awesome work! I came back today to see if I could pick up on this project and to my delight it's already complete. I've archived the repo I had started scaffolding for this effort and pointed over to the https://github.com/delneg/source-map-sharp repo for now. Again great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Nagareyama
  
Done
Development

Successfully merging a pull request may close this issue.

8 participants