Victory: Rename `prop-types.js` to `custom-prop-types.js` in victory-core/src/victory-util

[michowski]

Feature Requests

Checklist

• [x] I've read through the Docs and Guides to make sure this functionality doesn't already exist

• [x] I've searched open issues to make sure I'm not opening a duplicate issue

Description

When running a build setup with less control over a config it is sometimes harder, if not impossible, to modify the way modules and their aliases are resolved. In my case, NextJS(webpack) + TS + ES modules, I've run into the following issue which costed me quite some investigation:

hot-dev-client.js:118 Uncaught (in promise) Error: prop_types__WEBPACK_IMPORTED_MODULE_0__.default.arrayOf is not a function

After opening packages/victory-core/src/victory-util I've found the culprit:

import PropTypes from "prop-types";
import CustomPropTypes from "./prop-types";

This (un)coincidental naming causes a conflict: for both imports, my webpack config resolves to the local ./prop-types.js file rather than the well-known npm lib prop-types. This has been confirmed by a good ol' console.log done for both PropTypes and CustomPropTypes.

My proposal is to simply rename prop-types.js to custom-prop-types.js and change the local import accordingly. This hopefully not intrusive change could potentially save hours of headaches to these 1 in a 1 000 000 folks like me, who spent a lot of time on adjusting their configs in all possible ways and still failed.

I'm using [email protected]. The following is my tsconfig.json:

{
  "compilerOptions": {
    "allowJs": true,
    "skipDefaultLibCheck": false,
    "baseUrl": "./",
    "rootDir": "./",
    "paths": {
      "*": ["*"]
    },
    "esModuleInterop": true,
    "jsx": "preserve",
    "lib": ["dom", "es2017"],
    "module": "esnext",
    "moduleResolution": "node",
    "noEmit": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "preserveConstEnums": true,
    "removeComments": false,
    "skipLibCheck": false,
    "sourceMap": true,
    "strict": true,
    "target": "es2015"
  },
  "exclude": ["server"]
}

and next.config.js

require('dotenv').config();

const webpack = require('webpack');
const withTypescript = require('@zeit/next-typescript');
const ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin');
const SVGSpritemapPlugin = require('svg-spritemap-webpack-plugin');

module.exports = withTypescript({
  generateEtags: false,
  webpack(config, options) {
    // Do not run type checking twice:
    if (options.isServer) config.plugins.push(new ForkTsCheckerWebpackPlugin());

    config.plugins.push(
      new SVGSpritemapPlugin({
        src: 'static/icons/*.svg',
        filename: '../static/icons.svg',
        prefix: 'icon-',
      })
    );
    config.plugins.push(new webpack.EnvironmentPlugin(process.env));
    config.resolve.modules = ['./', 'node_modules'];

    return config;
  },
});

[ryan-roemer]

That's confusing, as:

import PropTypes from "prop-types";
import CustomPropTypes from "./prop-types";

is totally valid and separate according to Node require semantics (followed by webpack in most cases). (Same thing could totally occur with from "foo" and from "./foo").

If possible, could you put together a minimal public repository with installation, build and error reproduction steps to help us dig in and diagnose more? Thanks!!!

[ryan-roemer]

And just noticed:

config.resolve.modules = ['./', 'node_modules'];

What happens if you change it to:

// COMMENT IT OUT
//
// ... OR ...
//
config.resolve.modules = ['node_modules'];

?

[michowski]

Thank you very much Ryan and my apologies for not stressing enough an important fact which you just mentioned:

is totally valid and separate according to Node require semantics (followed by webpack in most cases). (Same thing could totally occur with from "foo" and from "./foo").

As it always happens, I've figured it out only right after posting this issue :) ... Your solution is the fix indeed: I had to remove './' and manually add all project-specific aliases:

config.resolve.modules = ['node_modules'];
config.resolve.alias = {
      api: path.resolve(__dirname, 'api/'),
      components: path.resolve(__dirname, 'components/'),
      constants: path.resolve(__dirname, 'constants/'),
      containers: path.resolve(__dirname, 'containers/'),
      lib: path.resolve(__dirname, 'lib/'),
      stores: path.resolve(__dirname, 'stores/'),
      types: path.resolve(__dirname, 'types/'),
    };

Thank you very very very much! PS Victory rocks!

[ryan-roemer]

Haha, no worries @michowski -- often the process of talking through an issue sheds light on the resolution, even if a bit different than what the original details might suggest.

Thanks for your kind words for Victory and happy hacking :)