huozhi / rollup-preserve-directives

This is a rollup plugin that helps preserve shebang and string directives.
https://npmjs.com/package/rollup-preserve-directives
34 stars 4 forks source link

fix: remove rollup warning & separate directives for each output chunk #2

Closed SukkaW closed 1 year ago

SukkaW commented 1 year ago

The PR does 2 things:

SukkaW commented 1 year ago

ping @huozhi

huozhi commented 1 year ago

Is that possible to use onLog to fix ignore the warning? editing the code is bit tricky if you already found that span in swc might not be super reliable. Another reason is what if other plugin also checks the directives?

SukkaW commented 1 year ago

Is that possible to use onLog to fix ignore the warning?

Probably not. The warning from rollup is intentional, to let library developers know they should take actions. rollup-swc-preserve-directives-plugin is just one of the possible action that library authors could take.

Also, the warning also shows if the plugin is working properly.

editing the code is bit tricky

rollup official plugins also use magic-string to manipulate code, so I guess it is safe: https://github.com/search?q=repo%3Arollup%2Fplugins%20magic-string&type=code

if you already found that span in swc might not be super reliable.

Actually it is a bug of the swc's JS API. And the swc team decides to ignore the issue, since in the next major version of swc (swc v2) there will be no swc.parse JS API at all anyway.

For now, we can use tests and snapshots to ensure the current workaround works reliably.

In the future PR, I will migrate away from swc.parse to use the rollup's built-in this.parse plugin API instead (which also has stable/reliable span).

Another reason is what if other plugin also checks the directives?

IMHO, the rollup-swc-preserve-directives-plugin should be added after every other plugin (especially after the swc/babel transformation).

In the future PR, I will add order: 'post' declaration to the transform hook : https://rollupjs.org/plugin-development/#build-hooks

huozhi commented 1 year ago

rollup official plugins also use magic-string to manipulate code, so I guess it is safe: github.com/search?q=repo%3Arollup%2Fplugins%20magic-string&type=code it's not related to magic string but we're deleting the comment

In the future PR, I will migrate away from swc.parse to use the rollup's built-in this.parse plugin API instead (which also has stable/reliable span).

that's also the purpose of this plugin that swc.parse is much faster than acorn parse and will have interpreter information.

Actually it is a bug of the swc's JS API. And the swc team decides to ignore the issue, since in the next major version of swc (swc v2) there will be no swc.parse JS API at all anyway.

can you share more where did you se it's going to be deprecated?

SukkaW commented 1 year ago

that's also the purpose of this plugin that swc.parse is much faster than acorn parse

The swc is only fast when it is inside the rust world. That's to say, when moving the AST struct from the rust world to the AST JSON in the js world, the swc becomes so slow that it actually becomes slower than the acorn (And that's why the swc team wants to remove the parse JS API in v2 in the first place).

Check out this benchmark (https://replit.com/@isukkaw/GoldenVividDebuggers), where the swc parse is about 40% to 60% slower than the acorn.

and will have interpreter information.

The hashbang/shebang/interpreter is very simple and can be easily ported:

In short, we don't really need swc to do this.

can you share more where did you see it's going to be deprecated?

https://github.com/swc-project/swc/issues/1366#issuecomment-900805854 https://github.com/swc-project/swc/issues/1366#issuecomment-902455836