-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix generated JS output to not insert indentation characters inside multiline `` strings. #26053
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 generated JS output to not insert indentation characters inside multiline `` strings. #26053
Conversation
b9bf767 to
be74046
Compare
sbc100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % comments
test/test_other.py
Outdated
| 'closure': (['--closure', '1'],), | ||
| }) | ||
| def test_multiline_string(self, args): | ||
| self.do_run_in_out_file_test('test_multiline_string.c', cflags=['--js-library', test_file('test_multiline_string.js')] + args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this test instead to test/test_jslib.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check
| if (text[i] == '\n') out += indent; | ||
| } | ||
| return text.replace(/\n/g, `\n${indent}`); | ||
| return out; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wonder at what point we should just acorn for this rather than adding complexity here? Or maybe we should just skip this process completely when the string contains any backticks at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I did already go through the trouble of implementing this logic.. so simplest to land this. If it turns out to be buggy in a way that is not seen by the harness, and is difficult to fix, then we can look at alternatives.
| @@ -0,0 +1,10 @@ | |||
| #include <stdio.h> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this files to test/jslib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check
…ultiline `` strings.
be74046 to
8967e46
Compare
E.g. a JS library function containing
would incorrectly be emitted as
(i.e.
"abc\n def\n ghi")when the output JS was generated, changing the contents of the multiline string.