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

scroll root parser evalMacros should escape dollar signs in the replacement string #157

Open
btheado opened this issue Jan 13, 2025 · 3 comments

Comments

@btheado
Copy link
Contributor

btheado commented Jan 13, 2025

As mentioned in this pldb issue, the djot page has messed up output.

I think the issue is the pldb example contains

$`p = mv`

and this line is using that in the regexp replacement string

Object.keys(macroMap).forEach(key => (codeAfterMacroSubstitution = codeAfterMacroSubstitution.replace(new RegExp(key, "g"), macroMap[key])))

According to MDN, the dollar sign followed by backtick has special meaning in the replacment string.

I think prior to using the replacement string (i.e. macroMap[key]), any dollar sign should be replaced with a double dollar sign.

@breck7
Copy link
Owner

breck7 commented Jan 13, 2025

Nice work!

Here's a simple repro:
https://try.scroll.pub/#scroll%0A%20buildTxt%0A%20%0A%20replace%20foo%20bar%0A%20foo%20foo%0A%20%0A%20replace%20problem%20%24%60%0A%20plainText%20a%20problem

buildTxt

replace foo bar
foo foo

replace problem $`
plainText a problem

@btheado
Copy link
Contributor Author

btheado commented Jan 14, 2025

Nice simplification! I wish I knew it was easily reproducible in a bare scroll context. Working via the pldb codebase was a bit hairy. It took a lot of work to simplify it down from that direction.

In order to build only djot.scroll in isolation, it didn't work to move it to a separate directory and build it. Well, it worked, but it still ended up still using the djot.scroll file in the concepts directory. I had to leave it in the concepts directory and remove all the other scroll files. I also had to remove all but the djot entry from pldb.json. Then I started paring down code from Computer.js.

A bit off topic for general scroll...I just wanted to share my experience.

@breck7
Copy link
Owner

breck7 commented Jan 14, 2025

Thank you thank you thank you for that hard work. I could simplify it only because you identified the macro issue.

PLDB is still pretty hacky (the Computer.js I want to eventually remove). And annoyingly slow. I'm working toward a cleanup that delivers 400% better perf. Hopefully tomorrow.

I also think what might be helpful in Scroll is to not just be able to add Parsers to a context, but to be able to remove them as well.

Perhaps something like:

outOfScope [list of parsers]

And then could turn off macros, etc.

like regulatory genes

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

No branches or pull requests

2 participants