Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Case-insensitive filesystem issue (macOS): Fraction.js/Complex.js files vs. fraction.js/complex.js NPM packages #2870

Closed
ryan-williams opened this issue Jan 2, 2023 · 10 comments

Comments

@ryan-williams
Copy link

Describe the bug

On a case-insensitive filesystem (I'm using macOS 12.1 with an APFS volume), something in node or webpack seems to get confused between Complex.js in this repo and the complex.js NPM package, and similarly Fraction.js vs. fraction.js on NPM.

error-screenshot

The warnings in the console above seem most relevant; more info below.

To Reproduce

On macOS (APFS Data Volume, case-insensitive)…

Clone ryan-williams/mathjs-test:

git clone https://github.com/ryan-williams/mathjs-test
cd mathjs-test
npm i

Or create repro from scratch:

# Create sample project, install mathjs
npx create-next-app mathjs-test --js --no-eslint --use-npm
cd mathjs-test
npm i --save mathjs

# Simple index.js invoking `fraction` or `complex`
cat >pages/index.js <<EOF
import {complex, fraction} from "mathjs"
export default function Home() {
  // Either of these lines causes an error
  // const n = complex(1, 1)
  const f = fraction(1, 2)
  return <div>yay</div>
}
EOF

Run next dev server, open in browser

PATH="${PATH}:node_modules/.bin"
next dev &
open http://127.0.0.1:3000

Dev console shows warnings

./node_modules/mathjs/lib/esm/type/complex/Complex.js
There are multiple modules with names that only differ in casing.
This can lead to unexpected behavior when compiling on a filesystem with other case-semantic.
Use equal casing. Compare these module identifiers:
* javascript/esm|/Users/ryan/c/mathjs-test/node_modules/mathjs/lib/esm/type/complex/Complex.js
    Used by 1 module(s), i. e.
    javascript/esm|/Users/ryan/c/mathjs-test/node_modules/mathjs/lib/esm/factoriesAny.js
* javascript/esm|/Users/ryan/c/mathjs-test/node_modules/mathjs/lib/esm/type/complex/complex.js
    Used by 2 module(s), i. e.
    javascript/esm|/Users/ryan/c/mathjs-test/node_modules/mathjs/lib/esm/type/complex/Complex.js
…
./node_modules/mathjs/lib/esm/type/fraction/Fraction.js
There are multiple modules with names that only differ in casing.
This can lead to unexpected behavior when compiling on a filesystem with other case-semantic.
Use equal casing. Compare these module identifiers:
* javascript/esm|/Users/ryan/c/mathjs-test/node_modules/mathjs/lib/esm/type/fraction/Fraction.js
    Used by 1 module(s), i. e.
    javascript/esm|/Users/ryan/c/mathjs-test/node_modules/mathjs/lib/esm/factoriesAny.js
* javascript/esm|/Users/ryan/c/mathjs-test/node_modules/mathjs/lib/esm/type/fraction/fraction.js
    Used by 2 module(s), i. e.
    javascript/esm|/Users/ryan/c/mathjs-test/node_modules/mathjs/lib/esm/type/fraction/Fraction.js

…and corresponding error

index.js?46cb:606 Uncaught TypeError: Object.defineProperty called on non-object
    at Function.defineProperty (<anonymous>)
    at createComplexClass.isClass (Complex.js?51b2:11:1)
    at assertAndCreate (factory.js?2286:35:1)
    at eval (pureFunctionsAny.generated.js?b0ab:12:55)
    at ./node_modules/mathjs/lib/esm/entry/pureFunctionsAny.generated.js (index.js?ts=1672683969845:4196:1)
    at options.factory (webpack.js?ts=1672683969845:673:31)
    at __webpack_require__ (webpack.js?ts=1672683969845:37:33)
    at fn (webpack.js?ts=1672683969845:328:21)
    at eval (mainAny.js:11:88)
    at ./node_modules/mathjs/lib/esm/entry/mainAny.js (index.js?ts=1672683969845:4185:1)
    at options.factory (webpack.js?ts=1672683969845:673:31)
    at __webpack_require__ (webpack.js?ts=1672683969845:37:33)
    at fn (webpack.js?ts=1672683969845:328:21)
    at ./node_modules/mathjs/lib/esm/index.js (index.js?ts=1672683969845:10038:75)
    at options.factory (webpack.js?ts=1672683969845:673:31)
    at __webpack_require__ (webpack.js?ts=1672683969845:37:33)
    at fn (webpack.js?ts=1672683969845:328:21)
    at eval (index.js:7:64)
    at ./pages/index.js (index.js?ts=1672683969845:49:1)
    at options.factory (webpack.js?ts=1672683969845:673:31)
    at __webpack_require__ (webpack.js?ts=1672683969845:37:33)
    at fn (webpack.js?ts=1672683969845:328:21)
    at eval (?44d9:5:16)
    at eval (route-loader.js?ea34:211:51)

error-screenshot

Works on Linux / in Docker

Running the same thing in Docker, on a Linux base image (with case-sensitive filesystem), works without these warnings or errors:

FROM node
RUN npx create-next-app mathjs-test --js --no-eslint --use-npm
WORKDIR mathjs-test
RUN npm i --save mathjs
COPY pages/index.js pages/index.js
EXPOSE 3000/tcp
ENV PATH="${PATH}:node_modules/.bin"
ENTRYPOINT ["next", "dev"]

Build, run, open:

docker build -t mathjs-test .
docker run --rm -d -p 3001:3000 mathjs-test
open http://127.0.0.1:3001

Page loads+renders without errors or warnings:

success-screenshot

Notes

  • I'm not sure if the issue is specific to next.js, that's just where I ran into it, and is the simplest way I know of to demonstrate it.
  • I originally ran into this instantiating a danfojs DataFrame.
    • This blocks using danfojs in next.js on my macbook…
    • I will move my local dev/deploy workflow into Docker, as a workaround.

Relevant versions

  • node v19.3.0
  • npm 9.2.0
  • mathjs 11.5.0
  • next 13.1.1
  • macOS 12.1
@josdejong
Copy link
Owner

hanks for the clear error report Ryan. I've run your demo on Windows (which is case-insensitive) and it works just fine there, but I don't have a Mac to try this out.

In the mathjs code base, there are the following files with similar naming:

'complex.js' // npm package
'src/type/complex/type/Complex.js'
'src/type/complex/function/complex.js'
'src/utils/complex.js'
'src/expression/embeddedDocs/construction/complex.js'

From a human point of view, it may be good to rename these files somehow to make this less confusing.

However from a technical point of view, all files do have a unique path, so there is no conflict at all. I do not understand why you get these warnings and errors. It sounds like a bug in the tool that generates this error. As if it interprets importing the npm library import Complex from 'complex.js' in the file src/type/complex/type/Complex.js as if it is a relative import to the file itself with wrong casing.

I expect the following minimal example to also throw an exception on Mac, maybe interesting to try out?

  • create a next.js project
  • install the npm package complex.js
  • create a file ./pages/Complex.js (mind the casing!) with the following contents:
    import Complex from 'complex.js'
    
    console.log('test complex.js', new Complex(2, 3))
  • import that file in the app

@josdejong
Copy link
Owner

I've created a PR with a workaround for this (I think) bug in Next.js: #2872. Do you think that will solve the issue?

@ryan-williams
Copy link
Author

ryan-williams commented Jan 5, 2023

Thanks for this @josdejong! I agree with everything you've said.

Updated+Renamed repro repo: ryan-williams/next.js-import-bug

Inspired by your suggested minimal example, I have a repro that now only involves complex.js and next.js, and doesn't involve mathjs or rely on ambiguity due to a case-insensitive filesystem.

pages/complex.js
// This should resolve to the `complex.js` NPM package, but
// next.js/webpack rewrites it as "./complex.js"
import Complex from 'complex.js'

const c = new Complex(11, 22)
if (!!c.props) {
    console.error('`Complex` class refers to a next.js page ❌', c, Complex)
} else if (c.re) {
    console.log("`Complex` class is correct ✅", c, Complex)
} else {
    console.error("`Complex` class not recognized:", c, Complex)
}

export default function Home() {
    return <div>yay</div>
}
  • I still see the issue on my macbook, but not in a Linux Docker image.
  • My next step is to raise it with next.js folks.

#2872?

I appreciate your PR #2872; I haven't had a chance to test it, but my gut is this isn't really a mathjs problem, so I don't want to push you to add hacks or workarounds unnecessarily here.

Feel free to move ahead with it if you think it makes sense, otherwise I will just update here as I learn more! Happy to close this in the meantime as well, up to you.

Thanks again, really appreciate your prompt attention to this 🙏

@kungfooman
Copy link

I don't know if it helps, but TypeScript (just use it via jsconfig.json in VSCode) has this option:

https://www.typescriptlang.org/tsconfig#forceConsistentCasingInFileNames

Force Consistent Casing In File Names - forceConsistentCasingInFileNames

TypeScript follows the case sensitivity rules of the file system it’s running on. This can be problematic if some developers are working in a case-sensitive file system and others aren’t. If a file attempts to import fileManager.ts by specifying ./FileManager.ts the file will be found in a case-insensitive file system, but not on a case-sensitive file system.

When this option is set, TypeScript will issue an error if a program tries to include a file by a casing different from the casing on disk.

That should help to detect this bug.

@ryan-williams
Copy link
Author

Interesting, thanks @kungfooman!

@benjamn also pointed out that adding a trailing slash to potentially ambiguous imports (e.g. NPM package names that could conceivably also be local file paths) seems to force importing from the package, as intended, e.g.

-import Complex from 'complex.js'
+import Complex from 'complex.js/'  // prevent erroneous circular import of ./Complex.js on case-insensitive filesystems; see https://github.com/vercel/next.js/issues/44603

at src/type/complex/Complex.js#L1, and similar at src/type/fraction/Fraction.js#L1:

-import Fraction from 'fraction.js'
+import Fraction from 'fraction.js/'  // prevent erroneous circular import of ./Fraction.js on case-insensitive filesystems; see https://github.com/vercel/next.js/issues/44603

It still seems better to identify/fix the bundler configs that are performing this incorrect optimization/resolution; I filed vercel/next.js#44603 about it.

Maybe I can test those trailing slashes and see if it fixes the issue here though.

@josdejong
Copy link
Owner

Thanks @ryan-williams for working out a bug report and minimal example!

Let's await the response on your bug report at next.js and see if it can be fixed in short term. If not, let's just merge my PR #2872 (we should be pragmatic).

@josdejong
Copy link
Owner

@ryan-williams shall we just merge my PR #2872 ? I do not see the issue being fixed at next.js soon.

@ryan-williams
Copy link
Author

Thanks for following up @josdejong; I feel like the "trailing slash" work-around I sketched above is a bit more direct than renaming the classes as in #2872. If it's ok with you, I am fine to wait and try test and PR that, for comparison.

The fact that no one else seems to be running into this or able to reproduce it also makes me want to wait and try to understand what's really happening…

@josdejong
Copy link
Owner

Ahh, yes, your workaround with the trailing slash is much better, I forgot about that 👍 .

I've committed your fix straight into the develop branch now via 078049e, let's see if that fixes stuff. (It's not yet tested nor published)

@josdejong
Copy link
Owner

o wow, I was too quick. The trailing slash gives problems with some of the build scripts: "Error Directory import '...\mathjs\node_modules\complex.js' is not supported resolving ES modules". And trying 'complex.js/complex.min.js' for example gives problems too when building. This needs some more investigation, help would be welcome. And if it's too much work I suggest we go for #2872 2872 anyway, it is straightforward and works.

I'll revert the commit for now.

josdejong added a commit that referenced this issue Jan 24, 2023
…ackage names `complex.js` and `fraction.js` with relative filenames"

This reverts commit 078049e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants