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

Support for __PURE__ annotations on tagged template literals #4035

Open
wereHamster opened this issue Apr 8, 2021 · 10 comments
Open

Support for __PURE__ annotations on tagged template literals #4035

wereHamster opened this issue Apr 8, 2021 · 10 comments

Comments

@lukastaegert
Copy link
Member

As this syntax is originally adapted from terser/uglifyjs, Rollup is trying to stick to their behaviour. Trying this in their REPL, it seems they do not support this for tagged template literals. I wonder if there is a deeper reason, maybe someone from their side can shed some light, e.g. @fabiosantoscode?

@wereHamster
Copy link
Author

I already opened an issue with terser: terser/terser#975

@fabiosantoscode
Copy link

fabiosantoscode commented Apr 11, 2021

Hello!

This is a touchy subject especially when keeping compatibility with UglifyJS.

/*#__PURE__*/ makeMacro`some code`("arg for some code")
/*#__PURE__*/ makeMacro("some thing")`Hello`

You can have two tools remove different parts of the code here.

My alternative idea

Create a new annotation /*#__PURETPL__*/ for template strings. Annotating something like the examples above could get hairy though and would probably involve parens

/*#__PURE__*/ ( /*#__PURETPL__*/ makeMacro`some code` )("arg for some code")
/*#__PURETPL__*/ ( /*#__PURE__*/ makeMacro("some thing") )`Hello`

Interested to hear what @kzc and @alexlamsl have to say on this, not only because they know more about annotations than me, but also because ideally this is also implemented in UglifyJS.

Side note:

If you'd like to mark any expression entirely pure, you can use this awkward workaround:

/*#__PURE__*/ (() => template`string`)()

@kzc
Copy link
Contributor

kzc commented Apr 11, 2021

Related discussion: mishoo/UglifyJS#4763 (comment)

I think a new pure annotation with a different spelling for each purpose would be confusing.

The main concern is an ambiguous pure annotation where the tag is a function call itself:

/*@__PURE__*/foo.bar(effect1())`baz ${effect2()}`, moo();

By the original uglify-js pure annotation rules, all this code would be retained since the annotation applies to foo.bar(effect1()) only - and the expression is used.

The question is whether a non-backwards compatible change should be made to transform the code fragment above to:

effect2(), moo();

Under this proposal the side effects in the tag would be dropped completely - as is the case with the function expression in a pure annotated function call under the original rules. Likewise, the side effects in the template component would be retained - just as function parameter side effects are retained in the original pure annotation rules.

However, retaining the side effects in the template part of the tagged template may be unexpected and possibly undesirable. A pure annotated IIFE, although awkward, does not have these issues.

Rather than extending /*@__PURE__*/ annotations to tagged templates I think the general problem of marking arbitrary expressions "pure" (technically "drop if unused") should be reexamined. Comments are not first class constructs in JS and make for poor syntax scaffolding. I still think using a call-like syntax __PURE__(expr) as mentioned in a previous discussion would be a better general solution as it is AST friendly and does not require parentheses in corner cases. The polyfill function __PURE__(x){return x} could be dropped or inserted by bundlers and minifiers as required.

I'm afraid that we will not reach consensus on this matter. This has all been hashed over before. But the status quo is not necessarily a bad thing. The awkward pure anotated IIFEs can readily be reduced away by optimizing minfiers and bundlers.

@XGHeaven
Copy link

@lukastaegert

I wonder if there is a deeper reason, maybe someone from their side can shed some light

It's useful for css-in-js library. It's important to tree-shaking unused tagged template. for example

// 'common-style' module
import { css } from 'emotion'

export const ShadowColor = 'rgba(0,0,0,.2)'
export const BorderColor = 'blue'
export const LightShadowStyle = css`box-shadow: ${ShadowColor}`
export const BorderStyle = css`border: ${BorderColor}`

// index.ts
import { LightShadowStyle } from 'common-style'

ReactDOM.render(<div css={LightShadowStyle}/>, document.body)

Expected: BorderColor & BorderStyle should not output.

@LongTengDao
Copy link
Contributor

LongTengDao commented Dec 1, 2021

Hi, @lukastaegert @fabiosantoscode

The obvious reason that Uglify not supporting PURE for tagged template call, is that, Uglify is a very old libary, which not intend to support es6...

Tagged template call is completely a syntax sugar for function call:

f`${effect()}`; // completely equal to f([ ... ],  effect())

Uglify also not support import(), because that's es6 too, does rollup and terser need to add a /*#__PUREIMT__*/ instead of /*#__PURE__*/import()? (Actually terser support /*#__PURE__*/import(), and rollup not support that, which is also non compatibility, and no discussion nor confuse like this issue.)

If we too care about "compatibility", then any new design will be not "compatibility", because maybe some user in some world, they write /*#__PURETPL__*/ as a normal comment (not annotation) just for fun (I mean private use seriously. for example, some one intend to mean ignore effect in all argument expression, while the others not)... It can never be "completely compatibility".

If we really care about "compatibility", I think the best way may be something like options.pureAnnotationAlsoForTaggedCall which is default disabled. That's really safe, and not make the source code being more and more like "coding in annotationsss".

So it's amazing for rollup and terser authors to confuse about this question... Am I right?


One opinion more, I think in this matter, the responsibility of bundler is bigger than minifier.

Because almost no one nowadays use minifier without after bundling step, minifier's tree shaking can almost do nothing left, after all, we cann't minify const a = /*#__PURE__*/f(); g(); return [ a, a ]; to g(); return [ f(), f() ]; by the hazy semantic "PURE".

@kzc
Copy link
Contributor

kzc commented Dec 1, 2021

The obvious reason that Uglify not supporting PURE for tagged template call, is that, Uglify is a very old libary, which not intend to support es6...

uglify-js now supports ES2020+.

rollup and esbuild were both tested against uglify-js' test cases.

So it's amazing for rollup and terser authors to confuse about this question... Am I right?

This is insulting. Minify tool authors are very familiar with bundling and its impact on minification. Indeed, I first implemented pure annotations in uglify-js to accommodate webpack and browserify.

we cann't minify const a = /#PURE/f(); g(); return [ a, a ]; to g(); return [ f(), f() ]; by the hazy semantic "PURE".

There's no haziness about the current semantics of pure annotations. uglify-js and its forks wouldn't perform that optimization because it would likely be less efficient making multiple calls instead of using the variable set to the return value of f().

Using comments to annotate code is not ideal because they are not first class citizens in ECMAScript as they are considered whitespace and do not survive code transformations (or even parentheses) very well in third party tools.

I recommend to not alter the semantics of /*@__PURE__*/, because it's call-based behavior is well known and uniform across uglify-js and its forks, rollup and esbuild. Just getting an uglify-js compatible pure annotation implementation working correctly in rollup was not trivial, and required many iterations, refinements and a lot of testing over many months.

@LongTengDao
Copy link
Contributor

LongTengDao commented Dec 1, 2021

Thanks for your responses to follow up this issuse!

I recommend to not alter the semantics of /*@__PURE__*/, because it's call-based behavior is well known

Emmm... Isn't "tagged call" a "call"? Making /*#__PURE__*/ also works on "tagged call", it seems not changing the sematics what it was designed for.

If uglify supports es6 now, and its /*#__PURE__*/ didn't updated to support the new syntax "tagged call", then I think this is a non
compatibility behaviour, if we have to talk about compatibility with a thing not existing before.

@LongTengDao
Copy link
Contributor

But worry about backwards compatible still make sense in the real world, let's discuss about the detials:

The main concern is an ambiguous pure annotation where the tag is a function call itself:

/*@__PURE__*/foo.bar(effect1())`baz ${effect2()}`, moo();

By the original uglify-js pure annotation rules, all this code would be retained since the annotation applies to foo.bar(effect1()) only - and the expression is used.

The question is whether a non-backwards compatible change should be made to transform the code fragment above to:

effect2(), moo();

By the original rule, the /*@__PURE__*/ written there make no sense, so of cause we can infer that the user not mean the original rule...

If it's really a problem which block the process, then maybe we can go ahead to support /*#__PURE__*/ on simple tagged template call?

/*#__PURE__*/fn``; // shaked
/*#__PURE__*/mod.fn``; // also shaked
/*#__PURE__*/fn()``; // keep the orignal rule to preserve, until there is a consensus, use below currently:
/*#__PURE__*/( fn() )``; // shaked -- really better enough than /*#__PURE__*/( () => fn()`` )(); because there could be hundreds of samples in front end project...

I think this won't be dangrous? And these helps most use cases easier.

@KTibow
Copy link

KTibow commented Nov 18, 2023

FYI this method works fine with Rollup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants