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(napi/parser): add MagicString #7529

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

Boshen
Copy link
Member

@Boshen Boshen commented Nov 28, 2024

Hold magic string instance on the Rust side for utf8 string manipulation.

Copy link

graphite-app bot commented Nov 28, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the C-enhancement Category - New feature or request label Nov 28, 2024
Copy link

codspeed-hq bot commented Nov 28, 2024

CodSpeed Performance Report

Merging #7529 will not alter performance

Comparing napi-parser-magic-string (d143ef8) with main (defaf4b)

Summary

✅ 30 untouched benchmarks

@yyx990803
Copy link

I like this - having this would actually allow Vue's compiler-sfc to replace @babel/parser and MagicString with just oxc-parser.

  • Performance-wise, instead of making napi calls on every op, we should probably buffer the operations on the JS side and lazy-apply them only when toString is actually called.

  • With the buffering, in Rolldown we can also expose this API (maybe with the AST already available via hook arguments) and allow plugin transform hooks to return an oxc ParserBuilder instance instead of strings so we don't even need to pass the finalized string to JS ever.

@overlookmotel
Copy link
Contributor

overlookmotel commented Dec 2, 2024

I believe the API this PR provides is:

const code = 'const s: String = "测试"';
const oxc = new ParserBuilder(code);
const {program} = oxc.parseSync({ sourceFilename: 'test.ts' });
const stringLiteral = program.body[0].declarations[0].init;
// Magic string manipulation
oxc.remove(stringLiteral.start + 1, stringLiteral.end - 1);
// Get altered code
const alteredCode = oxc.toString();

How about returning the magic string from parseSync instead of passing in a ParserBuilder?

const code = 'const s: String = "测试"';
const {program, magicString} = parseSync(code, { sourceFilename: 'test.ts', magicString: true });
const stringLiteral = program.body[0].declarations[0].init;
// Magic string manipulation
magicString.remove(stringLiteral.start + 1, stringLiteral.end - 1);
// Get altered code
const alteredCode = magicString.toString();

i.e. You choose whether you want a MagicString to be returned from parseSync or not with an option, following same pattern as how you choose if you want a source map or not.

Builder patterns are quite rusty, but personally I think this is more idiomatic for a JS API.

@Boshen
Copy link
Member Author

Boshen commented Dec 2, 2024

I believe the API this PR provides is:

const code = 'const s: String = "测试"';
const oxc = new ParserBuilder(code);
const {program} = oxc.parseSync({ sourceFilename: 'test.ts' });
const stringLiteral = program.body[0].declarations[0].init;
// Magic string manipulation
oxc.remove(stringLiteral.start + 1, stringLiteral.end - 1);
// Get altered code
const alteredCode = oxc.toString();

How about returning the magic string from parseSync instead of passing in a ParserBuilder?

const code = 'const s: String = "测试"';
const {program, magicString} = parseSync(code, { sourceFilename: 'test.ts', magicString: true });
const stringLiteral = program.body[0].declarations[0].init;
// Magic string manipulation
magicString.remove(stringLiteral.start + 1, stringLiteral.end - 1);
// Get altered code
const alteredCode = magicString.toString();

i.e. You choose whether you want a MagicString to be returned from parseSync or not with an option, following same pattern as how you choose if you want a source map or not.

Builder patterns are quite rusty, but personally I think this is more idiomatic for a JS API.

I can't make your suggestion work without cloning the string

#[napi]
pub struct ParserBuilder {
    cell: ParserBuilderImpl,
}

self_cell!(
    struct ParserBuilderImpl {
        owner: String,

        #[covariant]
        dependent: MagicString,
    }
);

MagicString is MagicString<'owner>

@overlookmotel
Copy link
Contributor

Ah OK. I don't know the internals. If we could make my suggestion work, would you prefer it as an API?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants