Rollup fails to bundle memoizee correctly

[IamTheHttp]

This package cannot be correctly bundled by Rollup.

When attempting to bundle, rollup will freeze some objects, making the package fail at runtime

This is related to the freeze property of rollup (https://rollupjs.org/guide/en/#outputfreeze) which causes imports under certain conditions to be sealed (I'm not sure why they do that).

For anyone who's interested, a fix can be achieved by adding output.freeze:false to the rollup config:

export default [{
  input: 'src.js',
  output: [{
    freeze: false, // <-- The guilty property
    file: "dist/index.js",
    format: 'iife'
  }],
  plugins:[
    commonjs(),
    resolve()
  ]
}];

Repro: https://github.com/IamTheHttp/meomizee-with-rollup.git

[IamTheHttp]

Unfortunately while the above fix does prevent runtime errors, the bundle is still unusable

As can be seen here in the bundle result, the if conditions are incorrect resulting in the function not to return a memoized value

https://github.com/IamTheHttp/meomizee-with-rollup/blob/master/dist/index.js#L2020

[medikoo]

@IamTheHttp great thanks for report.

Still note it's not an issue in this package itself, but rather issue specific to rollup bundler, which may have some limitations, have you tried to report it over there?

[IamTheHttp]

Hey @medikoo, I somewhat disagree that it's a rollup issue, but it's definitely related to rollup!

This issue has two parts

1. Object.freeze build error - to be solved by freeze:false as I've done, and shown in their issue/PR Question/Suggestion about Object.freeze with namespaces rollup/rollup#1693 / Adds freeze option for opting-out of Object.freeze usage rollup/rollup#1696
2. The treatment of a conditional require with side-effects.

The first bullet has been dealt by the rollup team and the addition of the freeze:boolean option.

The second bullet is a bit more complicated, consider this:

// index.js
if (typeof window === 'object') require('./sideeffect');

// sideeffect.js
console.log('side effect');

// Completely tree shaken off, since it's not used in index.js anywhere
module.exports = {
  some: 'property'
}

The build output of rollup is:

// dist/index.js
(function () {
  'use strict';

  console.log('side effect');

  if (typeof window === 'object')

  var src = {

  };

  var src$1 = src;

  return src$1;

}());

This looks like an open (and closed) issue with rollup commonJS plugin rollup/rollup-plugin-commonjs#424

This doesn't look like it will be fixed by the plugin team.
I think it's relatively easy to fix this behaviour, maybe we can require without conditions, and add the condition to the required module.

Alternatively, we can offer a pre-build package (by webpack or other bundlers) that will just work for anyone else (Since the problem is the bundling).

As a library author myself, I think it's best to leave as little to chance as possible.
What do you think?

[medikoo]

Hey @medikoo, I somewhat disagree that it's a rollup issue, but it's definitely related to rollup!

@IamTheHttp this is a perfectly valid JS package, using just pure JS (no deprecated features), and plain CJS require syntax (without relying on any rarely used meta methods, which some bundlers may have problem with)

If bundler cannot package such package well, it means there's a limitation on bundler side, and not in a package. I hardly see it can be otherwise.

In this given case, if I understand correctly, some rollup specific optimizations when applied interfere with a package integrity so it breaks. It's a thing that should be discussed on Rollup ground and not here imo.

Note that rollup doesn't constitute any industry standard here to be followed, instead it is expected to follow the standards which are followed by package as this one.

[IamTheHttp]

Completely agreed, my only comment was about the package itself wanting to be accessible by others.

Yes, this is probably an issue with rollup's commonjs plugin, however, the fix is so incredibly simple (and to my personal opinion, slightly improves the code quality) that I'm not sure why we wouldn't want to include it.

Would you be open to a PR on this matter?
What I had in mind was to simply be more explicit in how the code is written, that's all.

[medikoo]

@IamTheHttp feel free to open the PR. If change is minimal I think I'll be fine to take it. Still I'd like to also understand well why it cannot be addressed on Rollup side,