medikoo / es5-ext

ECMAScript extensions (with respect to upcoming ECMAScript features)
ISC License
168 stars 81 forks source link

Remove excess `exports` statement #96

Closed dkamyshov closed 4 years ago

dkamyshov commented 4 years ago

Hi, medikoo!

There are weird export statements in the following files:

They are weird in a sense that everywhere else export is done via just module.exports.

The issue here is that Node is unable to handle this type of export if the file is being executed in a vm context, see this issue: https://github.com/nodejs/help/issues/2094

I use linaria in my project (linaria basically parses and executes files and all of their dependencies inside vm contexts to determine the content of CSS files during build), so whenever there is an import from a file that somehow (in my case: through memoizee, which depends on es5-ext) imports custom/error.js (or other mentioned files), linaria fails.

See this example:

// library.js
import memoizee from "memoizee/weak";

export const f = memoizee((/* ... */) => {
  /* ... */
});

export const colors = {
  RED: "red"
};

// Component.js
import styled from "linaria/react";
import { colors } from "./library.js";

const Component = styled.div`
  color: ${colors.RED};
`;

This will fail.

dkamyshov commented 4 years ago

Here is the reproduction: https://github.com/dkamyshov/es5-ext-linaria-vm-reproduction

Just run yarn.

medikoo commented 4 years ago

@dkamyshov Great thanks for PR

The issue here is that Node is unable to handle this type of export if the file is being executed in a vm context, see this issue: nodejs/help#2094

Technically this bug is not really related, as if we want to run CJS module in vm context, it should be done via additional function wrapper (it's how Node.js internally runs modules, see: https://github.com/nodejs/node/blob/efce655c0f1671d0e86b5c89092ac93db983ef94/lib/internal/modules/cjs/loader.js#L175-L176).

So technically you've discovered a bug in linaria, which I believe is worth reporting there.

Still, I agree that relying on exports is error-prone (when module.exports is re-assigned). So it's better in all cases to rely on module.exports. Hence I'm fine with this PR :)

dkamyshov commented 4 years ago

@medikoo thanks for pointing this out. I'll sure send a PR to linaria :)