fresh icon indicating copy to clipboard operation
fresh copied to clipboard

commonjs transform is invalid for npm:qs

Open CertainLach opened this issue 5 months ago • 3 comments

Given the following code based on /home/lach/.cache/deno/npm/registry.npmjs.org/qs/6.14.0/lib/formats.js

var replace = String.prototype.replace;
var percentTwenties = /%20/g;

var Format = {
  RFC3986: 'RFC3986'
};

module.exports = {
  'default': Format.RFC3986,
  formatters: {
    RFC3986: function (value) {
      return String(value);
    }
  },
  RFC3986: Format.RFC3986
};

Fresh produces the following:

+   var exports = {},
+     module = {};
+   Object.defineProperty(module, "exports", {
+     get() {
+       return exports;
+     },
+     set(value) {
+       exports = value;
+     }
+   });
    var replace = String.prototype.replace;
    var percentTwenties = /%20/g;
    var Format = {
      RFC3986: 'RFC3986'
    };
    module.exports = {
      'default': Format.RFC3986,
      formatters: {
        RFC3986: function (value) {
          return String(value);
        }
      },
      RFC3986: Format.RFC3986
    };
+   var _formatters = exports.formatters;
+   var _RFC = exports.RFC3986;
+   export { _formatters as formatters, _RFC as RFC3986 };
+   const _default = exports.default ?? exports;
+   _default.formatters = _formatters;
+   _default.RFC3986 = _RFC;
+   export default _default;
+   export var __require = exports;

That crashes with the following error: TypeError: Cannot create property 'formatters' on string 'RFC3986'

Altering the default export like this doesn't seem to be right, perhaps this part:

const _default = exports.default ?? exports;
_default.formatters = _formatters;
_default.RFC3986 = _RFC;

Should be rewritten as

let _default;
if ('default' in exports) {
  _default = exports.default;
} else {
  _default = exports;
  _default.formatters = _formatters;
  _default.RFC3986 = _RFC;
}

instead.

CertainLach avatar Sep 25 '25 14:09 CertainLach

On second thought, isn't the correct rewrite here is just

let _default;
if ('default' in exports) {
  _default = exports.default;
} else {
  _default = module.exports;
}

There is no need to re-assign all the exports again, ~~and this implementation doesn't work correctly with module.exports = 1, which would be available as default in node.js esm support implementation~~ didn't notice the defined setter on module object, only the value setting should be dropped

CertainLach avatar Sep 25 '25 16:09 CertainLach

Should also check typeof of exports then

CertainLach avatar Sep 25 '25 16:09 CertainLach

Implemented the updated solution: https://github.com/denoland/fresh/pull/3494/commits/3e846db9f1b2287f10ac75e71e6844fec2e44a3e

It is included in the same PR.

CertainLach avatar Sep 25 '25 16:09 CertainLach