metal / metal.js

Build UI components in a solid, flexible way
http://metaljs.com
Other
229 stars 59 forks source link

Consider removing `NODE_ENV` check from `isServerSide` #359

Open zenorocha opened 6 years ago

zenorocha commented 6 years ago

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

robframpton commented 6 years ago

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