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

feat(es/ast): Add raw to Str #4071

Merged
merged 62 commits into from
Mar 22, 2022

Conversation

alexander-akait
Copy link
Collaborator

@alexander-akait alexander-akait commented Mar 17, 2022

Description:

feat - add raw property for StringLiteral

Also:

  • fix JSX with only \n in props
  • improve perf for normal printing strings + print them as they were written by developer
  • simplify TemplateLiteral - do not store StringLiteral (use JsWord), we just duplicate AST node, but it is unnecessary

Note - there is another improvement and simplification in TemplateLiteral, but I will do it in another PR We need do it here to better rewrite code
Also we should add raw for numbers too, so we will support linting 1_000_000 ad also will print numbers as they were written by developer

@ArturAralin after this we can simplify quotes rule, no need to look at original code (it is slow)

BREAKING CHANGE:

Yes

Related issue (if exists):

No

Also:

  • we have duplicate IsDirective in parser and swc_ecma_utils

@@ -31,8 +31,7 @@ macro_rules! quote_str {
$crate::swc_ecma_ast::Str {
span: $span,
value: $s.into(),
has_escape: false,
kind: Default::default(),
raw: $s.into(),
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't we need ' or "?

@alexander-akait alexander-akait force-pushed the feat-improve-ast-strings branch 13 times, most recently from 73238f9 to 34c53c2 Compare March 19, 2022 22:36
@@ -1 +1 @@
var foo = "\xa0";
var foo = "\u{a0}";
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong as the target is es5.

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Thank you so much!

kind: StrKind::Normal {
contains_quote: false,
},
raw: Some(("\"".to_owned() + &v.to_string() + "\"").into()),
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be bad for performance.

I think

raw: Some(format!("\"{}\"", v)),

would work

Copy link
Member

Choose a reason for hiding this comment

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

You can manually use String for better performance, but I don' think it will have a huge didfference.
(I mean String::push & String::push_str)

}

impl Take for Str {
fn dummy() -> Self {
Str {
span: DUMMY_SP,
value: js_word!(""),
has_escape: Default::default(),
kind: Default::default(),
raw: Some("\"\"".into()),
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be None

}

impl Take for TplElement {
fn dummy() -> Self {
TplElement {
span: DUMMY_SP,
tail: Default::default(),
cooked: Take::dummy(),
raw: Take::dummy(),
cooked: Some("".into()),
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

kind: StrKind::Normal {
contains_quote: false,
},
raw: Some(format!("\"{}\"", v).into()),
Copy link
Member

Choose a reason for hiding this comment

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

I think you missed this?

Copy link
Member

Choose a reason for hiding this comment

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

Is having this better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kdy1 Hm, do you mean

You can manually use String for better performance, but I don' think it will have a huge didfference.
(I mean String::push & String::push_str)

Just create String::new()? I missed that because it is recovery mode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in real code this will not happen... but I can refactor to using String

Copy link
Member

Choose a reason for hiding this comment

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

I mean something like

{

let mut buf = String::new();
// ...
}

(I'm afk now)

@@ -1 +1 @@
console.log("\ud83d\ude00", "\ud83d@\ude00");
console.log("\\\0ud83d\\\0ude00", "\\\0ud83d@\\\0ude00");
Copy link
Member

Choose a reason for hiding this comment

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

I think this is wrong, as the input was

console.log("\ud83d" + "\ude00", "\ud83d" + "@" + "\ude00");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kdy1 I see... They are surrogates pairs... Looking for workaround

condition(), (x = 20);
z || condition(), (x = "fuji");
x = (condition(), "foobar");
x = y ? "foo" : "foo";
Copy link
Member

Choose a reason for hiding this comment

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

This should not be modified

@alexander-akait
Copy link
Collaborator Author

@kdy1 Fixed 👍

@@ -1,7 +1,7 @@
import { m } from "../index.f66dda46.js";
import { u as useLang, a as useTitleTemplate, b as useTitle, c as useMeta, d as useLink } from "./hoofd.module.6c5395cb.js";
function MetaTags() {
return useLang("nl"), useTitleTemplate("%s | \u{1F4AD}"), useTitle("Welcome to hoofd"), useMeta({
return useLang("nl"), useTitleTemplate("%s | 💭"), useTitle("Welcome to hoofd"), useMeta({
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be problematic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm, it was original code, so we keep them as is, original code:

useTitleTemplate("%s | 💭")

Because target is not es5, it will not be problem

kdy1
kdy1 previously approved these changes Mar 22, 2022
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

LGTM.
Thank you so much for the hard work.


swc-bump:

  • swc_ecma_ast --breaking

@kdy1 kdy1 changed the title Feat improve ast strings feat(es/ast): Add raw to Str Mar 22, 2022
@kdy1 kdy1 added this to the v1.2.160 milestone Mar 22, 2022
@kdy1 kdy1 enabled auto-merge (squash) March 22, 2022 07:52
@kdy1 kdy1 merged commit 634d732 into swc-project:main Mar 22, 2022
@alexander-akait alexander-akait deleted the feat-improve-ast-strings branch March 22, 2022 10:27
@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants