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): introduce experimental MagicString #7529

Merged
merged 1 commit into from
Dec 10, 2024

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
@Boshen Boshen force-pushed the napi-parser-magic-string branch from 5589f42 to d143ef8 Compare November 28, 2024 16:46
Copy link

codspeed-hq bot commented Nov 28, 2024

CodSpeed Performance Report

Merging #7529 will not alter performance

Comparing napi-parser-magic-string (4bd3d10) with main (2e69720)

Summary

✅ 29 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?

@Boshen Boshen force-pushed the napi-parser-magic-string branch from d143ef8 to 763aebd Compare December 9, 2024 14:59
@Boshen Boshen force-pushed the napi-parser-magic-string branch 2 times, most recently from 5ca3be0 to 8df1102 Compare December 10, 2024 08:20
@Boshen Boshen marked this pull request as ready for review December 10, 2024 08:20
@Boshen Boshen force-pushed the napi-parser-magic-string branch 2 times, most recently from b37a69d to ecf7b65 Compare December 10, 2024 08:27
@Boshen Boshen changed the title feat(napi/parser): add MagicString feat(napi/parser): introduce experimental MagicString Dec 10, 2024
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Dec 10, 2024
Copy link
Member Author

Boshen commented Dec 10, 2024

Merge activity

  • Dec 10, 3:32 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Dec 10, 3:33 AM EST: A user added this pull request to the Graphite merge queue.
  • Dec 10, 3:37 AM EST: A user merged this pull request with the Graphite merge queue.

Hold magic string instance on the Rust side for utf8 string manipulation.
@Boshen Boshen force-pushed the napi-parser-magic-string branch from 3dca097 to 4bd3d10 Compare December 10, 2024 08:33
@graphite-app graphite-app bot merged commit 4bd3d10 into main Dec 10, 2024
26 checks passed
@graphite-app graphite-app bot deleted the napi-parser-magic-string branch December 10, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants