coralproject / talk

A better commenting experience from Vox Media
https://coralproject.net
Other
1.89k stars 355 forks source link

add file-loader as a dev-dependency in client #4499

Closed oliver-dvorski closed 7 months ago

oliver-dvorski commented 7 months ago

What does this PR do?

Fixes https://github.com/coralproject/talk/issues/4105

These changes will impact:

What changes to the GraphQL/Database Schema does this PR introduce?

None

Does this PR introduce any new environment variables or feature flags?

No

If any indexes were added, were they added to INDEXES.md?

How do I test this PR?

Were any tests migrated to React Testing Library?

No

netlify[bot] commented 7 months ago

Deploy Preview for gallant-galileo-14878c canceled.

Name Link
Latest commit 5fce45ba3f9b3e0339b0db6c527ce1f6366e12d2
Latest deploy log https://app.netlify.com/sites/gallant-galileo-14878c/deploys/65afafce1abfd70008c85363
nick-funk commented 7 months ago

Thank you for taking the time to submit a PR!

A few of the items in your testing instructions caught my eye and I was hoping to point you in the right direction:

  1. You've indicated to use npm i --legacy-peer-deps. This is likely due to the fact you are still using the legacy node 14. If you check the package.json you'll notice that it states the following:

    "engines": {
        "node": "^18.16.0",
        "npm": "8.0.0"
     },

    This means you need to use Node 18.16.0 and npm 8.0.0. I'd recommend using nvm to install this (https://github.com/nvm-sh/nvm).

    With that installed, you should no longer need to use the --legacy-peer-deps flag.

  2. I see you're wanting us to add in the file-loader package. We don't use the file-loader package because it runs into a memory issue on macOS/linux and instead we use a more memory safe tooling from Meta called watchman (https://facebook.github.io/watchman/). If you install this, you won't need the file-loader package.

    This can be brought in via a simple brew install watchman if you have homebrew (https://brew.sh/) installed.

I hope that helps resolve your development issues!

oliver-dvorski commented 7 months ago

Thank you for taking the time to help me, @nick-funk. I appreciate that.

You are completely correct. I managed to eliminate both issues. But not by upgrading node or installing Watchman, I already have Node 18 and I've got Watchman installed. The issue went away when I downgraded npm from 8.19.4 to 8.0.0.

However, this doesn't remove the issue which is being reported in newer versions of npm. The client requires React 18 and @giphy/react-components@5.4.0 which requires React 16.10.2 - 17. But that's an issue for another pull request.

The topic of this PR is the webpack file-loader. I understand that you're using Watchman to get notified about file changes, but my understanding is that the webpack file-loader has nothing to do with tracking file changes. It's a file processing utility. If you take a look at this line:

https://github.com/coralproject/talk/blob/6d621e4e755a84f077e1976cd0b36abe6314a99b/client/src/core/build/createWebpackConfig.ts#L528

you'll notice that Coral does have a direct dependency on the package. It is being installed by npm 8.0.0 because it comes with url-loader@4.1.1, which is listed in the dev-dependencies. So it's there by chance and Coral does depend on it. If we want to be able to update npm in the future, this package has to be listed as a dependency, or we need to break the dependency on it.

I appreciate your time and work very much and I do hope we can find a solution for this issue.

ihardyslide commented 7 months ago

I can confirm I have the same issue since Coral v7.4.4. I am currently upgrading step by step from 7.3.0 to latest version. Node: 18.17.0 NPM: 8.19.4

Downgrading NPM to 8.0.0 did not help in my case.