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

Lots of whitespace differences introduced by endo bundler #926

Open
erights opened this issue Nov 3, 2021 · 11 comments · May be fixed by #2444
Open

Lots of whitespace differences introduced by endo bundler #926

erights opened this issue Nov 3, 2021 · 11 comments · May be fixed by #2444
Assignees
Labels
debugging support devex developer experience endo p1 A temporary marker for highly motivated changes to be marked as priority 1 on for Agoric Engineering

Comments

@erights
Copy link
Contributor

erights commented Nov 3, 2021

On the right is the original lockdown-shim.js. On the left is the corresponding portion of lockdown.umd.js aligned for a visual line by line comparison. There are lots of little divergences. I do not remember seeing these divergences when we were using some previous bundler (rollup?).

These differences will impede the debugging experience. It means that at least the columns are different, and so we cannot simply sourceURL the original source file to appear in the debugger in lieu of the bundled source.

image

Related issues:

@kriskowal
Copy link
Member

We discussed this and resolved that this probably can be addressed by patching the babel-generator code that catches up with the cursor of the next written AST, such that it advances the column in addition to the line.

@kriskowal
Copy link
Member

We also discussed trying to preserve untouched subtrees of the AST by reusing the original source. This is possible but more invasive. It requires annotating which nodes were “touched” and modifying bundle-source to attend to that detail.

@kriskowal kriskowal added the endo label Jan 27, 2022
@pyramation
Copy link

I would encourage the use of source maps vs. expecting babel to allow customization of whitespace:

https://github.com/evanw/node-source-map-support
https://github.com/chocolateboy/babel-plugin-source-map-support

If you integrate source maps correctly, all debugging should theoretically use the source code files instead of the transpiled code in stack traces.

If possible, I'd encourage and suggest finding a path that is not dependent on comments or whitespace output of babel. As far as I understand the deparser (babel/generate) or the transformer, whitespace (and comments) are two areas that I've empirically found that babel doesn't address perfectly.

Given the dependency on babel (which is the industry standard), if sourcemaps don't solve this issue, I'd suggest making an issue in babel. Looks like it has been brought up before as early as 2015, but has been closed babel/babel#497

@pyramation
Copy link

also, looking more deeply into the original babel issue I found a linked issue that helped me find this library: https://github.com/js-cst-tokens/cst-tokens

they are not using ASTs, but rather, CSTs which appear to preserve whitespace. I would still think sourcemaps are the better path, but figured I'd share this link.

@erights
Copy link
Contributor Author

erights commented Sep 5, 2022

Thanks! If the cst works (it looks like a great find!), I prefer it to the sourcemap path. With sourcemaps, the programmer is often looking at their original source text which can differ from the code actually being executed in devious ways. Even with the cst, I'd expect programmers to often be looking at their original source test, via sourceURL rather than text being executed. But, since these are supposed to differ only on import and export,

  • programmers can avoid the sourceURL view and have essentially the same experience
  • differences on lines other than import and export can be trivially caught by a diff.

CST FTW! Thanks much. I hope this works!

All that said, if we do sourcemaps, we can easily write a checker to ensure that the pre vs post source text differs only on import, export, and whitespace. We should probably do that anyway. Once that's verified, debugging through a sourcemap view would be safe.

@erights
Copy link
Contributor Author

erights commented Sep 5, 2022

Actually, the current babel output seems to differ on more than whitespace, for example unnecessary parens, so that a correspondence verifier for babel output is no longer such a trivial exercise. This pushes harder for us to convert to CSTs.

@pyramation
Copy link

makes sense! From what I can tell, the intention of CSTs is more aligned with your goals.

From what I can see on the readme, the flow should be somewhat similar to babel. So if you guys have a babel transformer for import and export, it should be decently portable to take a crack at using CSTs via these newly discovered libraries.

Keep me in the loop!

@erights
Copy link
Contributor Author

erights commented Sep 6, 2022

makes sense! From what I can tell, the intention of CSTs is more aligned with your goals.

From what I can see on the readme, the flow should be somewhat similar to babel. So if you guys have a babel transformer for import and export,

We do!

it should be decently portable to take a crack at using CSTs via these newly discovered libraries.

Keep me in the loop!

Will do. Much appreciated.

@erights erights self-assigned this Dec 24, 2022
@erights
Copy link
Contributor Author

erights commented Dec 24, 2022

See also #1044

@kriskowal
Copy link
Member

I’ve threaded source maps into a per-user cache as of #1689. This is as much as we can do to improve debugging a module through the module-to-program transform in our shim until we hear back from @nicolo-ribaudo.

@erights
Copy link
Contributor Author

erights commented Nov 16, 2023

Another one. In order to send a breakpoint in the handle method shown in my source on the left panel, I waited until the debugger took me into the corresponding bundled file shown on the right, line them up (which is why there's a very long first line scrolled off the top on the right). Fortunately, the line numbers do correspond accurately, so with these lined up I can interact with the debugger on the right pane while looking at the left pane.

Thus, the difficulty due to the loss of horizontal whitespace can be coped with awkwardly. What's harder is getting to the code in the right hand pane. The only way I've done it is to wait until I can step into it in the debugger, which requires following execution that reaches there somehow. This does not enable the normal debug experience of navigating to a position in a source file and placing a breakpoint ahead of time.

image

@kriskowal kriskowal added kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024 devex developer experience p1 A temporary marker for highly motivated changes to be marked as priority 1 on for Agoric Engineering labels Jan 10, 2024
@aj-agoric aj-agoric removed the kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024 label Jan 31, 2024
@kriskowal kriskowal linked a pull request Sep 3, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debugging support devex developer experience endo p1 A temporary marker for highly motivated changes to be marked as priority 1 on for Agoric Engineering
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants