react-financial / react-financial-charts

Charts dedicated to finance.
https://react-financial.github.io/react-financial-charts/
MIT License
1.2k stars 211 forks source link

Generated package imports missing .js file extension #606

Closed adamhwang closed 1 year ago

adamhwang commented 2 years ago

I'm submitting a...

What is the current behavior

Create a new project npx bug-demo create-next-app --ts --use-npm

Add react-financial-charts, and add a chart to the home page

Running the project npm start dev throws the following runtime error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.\node_modules\@react-financial-charts\annotations\lib\Annotate' imported from .\node_modules\@react-financial-charts\annotations\lib\index.js

What is the expected behavior

Package imports should be built with .js

Please tell us about your environment

  "name": "bug-demo",
  "private": true,
  "scripts": {
    "dev": "next dev",
    "build": "next build",
    "start": "next start",
    "lint": "next lint"
  },
  "dependencies": {
    "d3-format": "^3.1.0",
    "d3-time-format": "^4.1.0",
    "next": "12.0.7",
    "react": "17.0.2",
    "react-dom": "17.0.2",
    "react-financial-charts": "^1.3.2"
  },
  "devDependencies": {
    "@types/d3-format": "^3.0.1",
    "@types/d3-time-format": "^4.0.0",
    "@types/node": "16.11.12",
    "@types/react": "17.0.37",
    "eslint": "8.4.1",
    "eslint-config-next": "12.0.7",
    "typescript": "4.5.2"
  }
}

Other information

Personally, I feel like this is a Typescript issue, but they seem adamant that this is by design and the resolution should be to add .js file extensions to all imports: https://github.com/microsoft/TypeScript/issues/40878

eberlitz commented 2 years ago

I have the same issue trying to use this library with Next.js, searching around I found this: https://github.com/vercel/next.js/issues/31974, which states that NodeJS ESM modules should have Mandatory file extensions. https://nodejs.org/api/esm.html#mandatory-file-extensions.

So it really seems that the distributed file sources should have the .js extension in its imports/exports.

Modules: ECMAScript modules | Node.js v17.3.0 Documentation
adamhwang commented 2 years ago

@eberlitz I have a PR to fix the file extensions, but next still has issues with ESM modules and webpack 5 still had issues with how this is packaged and I was getting a bunch of Cannot read property 'isRequired' of undefined based on the PropTypes. I was finally able to get react-financial-charts to work with next 11 & 12 without any changes to the latest release (1.3.2) using the following next config compose:

const withTM = require('next-transpile-modules')([
    'd3-array',
    'd3-format',
    'd3-time',
    'd3-time-format',
    'react-financial-charts',
    '@react-financial-charts/annotations',
    '@react-financial-charts/axes',
    '@react-financial-charts/coordinates',
    '@react-financial-charts/core',
    '@react-financial-charts/indicators',
    '@react-financial-charts/interactive',
    '@react-financial-charts/scales',
    '@react-financial-charts/series',
    '@react-financial-charts/tooltip',
    '@react-financial-charts/utils',
]);

/** @type {import('next').NextConfig} */
const nextConfig = {
    reactStrictMode: true,
};

module.exports = withTM(nextConfig);

You can exclude the d3 stuff depending on if you're using that or not.

fasmat commented 2 years ago

I'm also affected by this issue. I cannot build @react-financial-chart/* packages because of this issue.

MeFisto94 commented 2 years ago

This is a "regression" from https://github.com/markmcdowell/react-financial-charts/commit/1359ac6e93d9638792c7bb478bba5fe1e5484a82 / #520, so reverting to 1.2.2 works as a workaround (especially if you don't use next but plain react-typescript).

Do keep in mind you also need to pin the sub-dependencies as seen two comments previously. Still, a proper solution is obviously much welcomed :)

qunaxis commented 2 years ago

Solution for create-react-app provided environment worked for me

  1. Install craco package (https://github.com/gsoft-inc/craco) if it didn't installed before
  2. Insert following config into craco.config.js file (or extend it):
module.exports = {
  webpack: {
    configure: {
      module: {
        rules: [
          {
            test: /\.m?js/,
            resolve: {
              fullySpecified: false,
            },
          },
        ]
      }
    }
  }
}

P.S. It's just a little overriding of default webpack 5 configuration provided by react-scripts@5.0.1 (my currently installed version).

GitHub
GitHub - gsoft-inc/craco: Create React App Configuration Override, an easy and comprehensible configuration layer for create-react-app
Create React App Configuration Override, an easy and comprehensible configuration layer for create-react-app - GitHub - gsoft-inc/craco: Create React App Configuration Override, an easy and compreh...
hakunin commented 2 years ago

@adamhwang Is there a way people can use your fork in the meantime? I tried including it as git repo but things don't seem to get resolved.

adamhwang commented 2 years ago

@adamhwang Is there a way people can use your fork in the meantime? I tried including it as git repo but things don't seem to get resolved.

You should be able to pull any branch into your project with npm, but I’m not sure how well you protect might work with that.

hakunin commented 2 years ago

@adamhwang Is there a way people can use your fork in the meantime? I tried including it as git repo but things don't seem to get resolved.

You should be able to pull any branch into your project with npm, but I’m not sure how well you protect might work with that.

Thanks, I did that. But the project gets added under the name of "root" and I'm unable to import anything from it. If anyone knows hows how to use this I'd appreciate any help.

I found the older D3 based projects are easier to get up and running so I will have to try react-stockcharts next.

markmcdowell commented 1 year ago

Module flag is now removed in the latest version.