metal.js icon indicating copy to clipboard operation
metal.js copied to clipboard

Consider removing `NODE_ENV` check from `isServerSide`

Open zenorocha opened this issue 7 years ago • 1 comments

Today I was trying to test our components using Jest's Node.js environment instead of JSDom.

/**
 * @jest-environment node
 */

import Component from 'metal-component';
import Alert from '../src/Alert';

describe('Alert.node', () => {
  it('should not fail on the server side', () => {
    const alert = Component.renderToString(Alert);
    expect(alert).not.toBeNull();
  });
});

After an hour trying to understand why Jest was throwing client-side errors when it shouldn't, I realized it's because they set NODE_ENV equals test [1] and we use that same value on isServerSide [2].

I don't know what was the reason behind that, but I think that line shouldn't exist. Sure, I could just set a different value to that environment variable, but as we move into having components that also compile on the server, I think it's important to have a isServerSide that we can rely on. Otherwise, people would just create their own like what was done on WeDeploy's Console.

[1] https://github.com/facebook/jest/issues/3370 [2] https://github.com/metal/metal.js/blob/master/packages/metal/src/coreNamed.js#L307

zenorocha avatar Feb 17 '18 01:02 zenorocha

If we make this change now it will break a lot of tests for the other product teams that are using Jest, including Clay.

robframpton avatar Feb 20 '18 16:02 robframpton