jwagner / simplex-noise.js

A fast simplex noise implementation in Javascript / Typescript.
MIT License
1.57k stars 130 forks source link

rollup complains about the `#__PURE__` annotations #66

Closed chase-moskal closed 2 weeks ago

chase-moskal commented 5 months ago

:wave: hey, love the library, i've used it for years for many projects, it's my go-to!

i noticed rollup is complaining about the #__PURE__ comments -- glancing at rollup's docs, i think it just wants these annotations positioned before the const?

i suspect a change like this might make both uglify and rollup happy? i'm using terser, which apparently supports these same kinds of annotations.

- const F2 = /*#__PURE__*/ 0.5 * (Math.sqrt(3.0) - 1.0);
+ /*#__PURE__*/ const F2 = 0.5 * (Math.sqrt(3.0) - 1.0);

rollup's warning, literally complaining about the meta-comment // these #__PURE__ comments help uglifyjs with dead code removal which is kinda funny

(!) A comment

"// these #__PURE__ comments help uglifyjs with dead code removal"

in "../toolbox/node_modules/simplex-noise/dist/esm/simplex-noise.js" contains an annotation that Rollup cannot interpret due to the position of the comment. The comment will be removed to avoid issues.
https://rollupjs.org/https://rollupjs.org/configuration-options/#pure
../toolbox/node_modules/simplex-noise/dist/esm/simplex-noise.js (29:0)
27:  SOFTWARE.
28:  */
29: // these #__PURE__ comments help uglifyjs with dead code removal
    ^
30: //
31: const F2 = /*#__PURE__*/ 0.5 * (Math.sqrt(3.0) - 1.0);

another rollup warning about the next line

(!) A comment

"/*#__PURE__*/"

in "../toolbox/node_modules/simplex-noise/dist/esm/simplex-noise.js" contains an annotation that Rollup cannot interpret due to the position of the comment. The comment will be removed to avoid issues.
https://rollupjs.org/https://rollupjs.org/configuration-options/#pure
../toolbox/node_modules/simplex-noise/dist/esm/simplex-noise.js (31:11)
29: // these #__PURE__ comments help uglifyjs with dead code removal
30: //
31: const F2 = /*#__PURE__*/ 0.5 * (Math.sqrt(3.0) - 1.0);
               ^
32: const G2 = /*#__PURE__*/ (3.0 - Math.sqrt(3.0)) / 6.0;
33: const F3 = 1.0 / 3.0;

this obviously is not a critical issue.

jwagner commented 5 months ago

👋 hey, love the library, i've used it for years for many projects, it's my go-to!

Thank you!

The next step here would be to run a quick experiment on what's needed with minifiers these days.

From a quick test with the terser REPL and uglifyjs it looks like

  const F2 =  0.5 * (/*#__PURE__*/Math.sqrt(3.0) - 1.0);

should work universally.

@chase-moskal Is there any easy way to test whether this would also make rollup happy?

dmnsgn commented 2 weeks ago

👋 hey, love the library, i've used it for years for many projects, it's my go-to!

Thank you!

The next step here would be to run a quick experiment on what's needed with minifiers these days.

From a quick test with the terser REPL and uglifyjs it looks like

  const F2 =  0.5 * (/*#__PURE__*/Math.sqrt(3.0) - 1.0);

should work universally.

@chase-moskal Is there any easy way to test whether this would also make rollup happy?

Rollup also has a REPL and your suggested fix seems to make it happy:

jwagner commented 2 weeks ago

Fix released in 4.0.2. :)