Detect dead code and unused dependencies (#1687)

* Use Knip to detect dead code

* Remove unused dependencies

* Move some dummy dependencies to devDependencies

* Improve types for Node Package

---------

Co-authored-by: Alexey Romanov <alexey.v.romanov@gmail.com>

Author: alexeyr-ci

.eslintignore

@@ -15,3 +15,5 @@ node_package/webpack.config.js
 **/public/packs*/*
 gen-examples
 bundle/
+# Can't get it working in CI
+knip.ts

.eslintrc

@@ -4,6 +4,7 @@ extends:
   - prettier/react
 
 plugins:
+  - import
   - prettier
 
 globals:

.github/workflows/lint-js-and-ruby.yml

@@ -47,9 +47,19 @@ jobs:
         yarn install --no-progress --no-emoji
     - name: Install Ruby Gems for package
       run: bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
-    - name: Linting of Ruby
+    - name: Lint Ruby
       run: bundle exec rubocop
-    - name: Linting of JS
+    - name: Install Node modules with Yarn for dummy app
+      run: cd spec/dummy && yarn install --ignore-scripts --no-progress --no-emoji
+    - name: Install Ruby Gems for dummy app
+      run: cd spec/dummy && bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+    - name: generate file system-based packs
+      run: cd spec/dummy && RAILS_ENV=test bundle exec rake react_on_rails:generate_packs
+    - name: Detect dead code
+      run: |
+        yarn run knip
+        yarn run knip --production
+    - name: Lint JS
       run: yarn start lint
     - name: Check formatting
       run: yarn start format.listDifferent

.github/workflows/main.yml

@@ -71,7 +71,7 @@ jobs:
     - name: generate file system-based packs
       run: cd spec/dummy && RAILS_ENV=test bundle exec rake react_on_rails:generate_packs
     - name: Build test bundles for dummy app
-      run: cd spec/dummy && rm -rf public/webpack/test && yarn build:rescript && RAILS_ENV=test NODE_ENV=test bin/${{ matrix.versions == 'oldest' && 'web' || 'shaka' }}packer
+      run: cd spec/dummy && rm -rf public/webpack/test && yarn run build:rescript && RAILS_ENV=test NODE_ENV=test bin/${{ matrix.versions == 'oldest' && 'web' || 'shaka' }}packer
     - id: get-sha
       run: echo "::set-output name=sha::$(git rev-parse HEAD)"
     - name: Save test webpack bundles to cache (for build number checksum used by rspec job)

.github/workflows/package-js-tests.yml

@@ -33,6 +33,9 @@ jobs:
       with:
         path: node_modules
         key: v5-package-node-modules-cache-${{ hashFiles('yarn.lock') }}
+    - name: run conversion script
+      if: matrix.versions == 'oldest'
+      run: script/convert
     - name: Install Node modules with Yarn for renderer package
       run: |
         yarn install --no-progress --no-emoji

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    react_on_rails (14.1.0)
+    react_on_rails (14.1.1)
       addressable
       connection_pool
       execjs (~> 2.5)

knip.ts

@@ -0,0 +1,78 @@
+import type { KnipConfig } from 'knip';
+
+const config: KnipConfig = {
+  // ! at the end means files are used in production
+  workspaces: {
+    '.': {
+      entry: ['node_package/src/ReactOnRails.ts!', 'node_package/src/ReactOnRails.node.ts!'],
+      project: ['node_package/src/**/*.[jt]s!', 'node_package/tests/**/*.[jt]s'],
+      babel: {
+        config: ['node_package/babel.config.js'],
+      },
+      ignoreBinaries: [
+        // Knip fails to detect it's declared in devDependencies
+        'nps',
+        // local scripts
+        'node_package/scripts/.*',
+      ],
+      ignoreDependencies: [
+        // Required for TypeScript compilation, but we don't depend on Turbolinks itself.
+        '@types/turbolinks',
+        // used in package-scripts.yml
+        'concurrently',
+        // The Knip ESLint plugin fails to detect these are transitively required by a config,
+        // though we don't actually use its rules anywhere.
+        'eslint-plugin-jsx-a11y',
+        'eslint-plugin-react',
+      ],
+    },
+    'spec/dummy': {
+      entry: [
+        'app/assets/config/manifest.js!',
+        'client/app/packs/**/*.js!',
+        // Not sure why this isn't detected as a dependency of client/app/packs/server-bundle.js
+        'client/app/generated/server-bundle-generated.js!',
+        'spec/fixtures/automated_packs_generation/**/*.js{x,}',
+        'config/webpack/{production,development,test}.js',
+        // Declaring this as webpack.config instead doesn't work correctly
+        'config/webpack/webpack.config.js',
+      ],
+      project: ['**/*.{js,cjs,mjs,jsx,ts,cts,mts,tsx}!', 'config/webpack/*.js'],
+      paths: {
+        'Assets/*': ['client/app/assets/*'],
+      },
+      ignoreBinaries: [
+        // Has to be installed globally
+        'yalc',
+        // Local binaries
+        'bin/.*',
+      ],
+      ignoreDependencies: [
+        // Knip thinks it can be a devDependency, but it's supposed to be in dependencies.
+        '@babel/runtime',
+        // There's no ReScript plugin for Knip
+        '@rescript/react',
+        // The Babel plugin fails to detect it
+        'babel-plugin-transform-react-remove-prop-types',
+        // This one is weird. It's long-deprecated and shouldn't be necessary.
+        // Probably need to update the Webpack config.
+        'node-libs-browser',
+        // The below dependencies are not detected by the Webpack plugin
+        // due to the config issue.
+        'css-loader',
+        'expose-loader',
+        'file-loader',
+        'imports-loader',
+        'mini-css-extract-plugin',
+        'null-loader',
+        'sass',
+        'sass-loader',
+        'sass-resources-loader',
+        'style-loader',
+        'url-loader',
+      ],
+    },
+  },
+};
+
+export default config;

node_package/src/buildConsoleReplay.ts

@@ -9,6 +9,7 @@ declare global {
   }
 }
 
+/** @internal Exported only for tests */
 export function consoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined, numberOfMessagesToSkip: number = 0): string {
   // console.history is a global polyfill used in server rendering.
   const consoleHistory = customConsoleHistory ?? console.history;

node_package/src/clientStartup.ts

@@ -7,6 +7,7 @@ import type {
   RenderFunction,
   Root,
 } from './types';
+import type { Context } from './context';
 
 import createReactOutput from './createReactOutput';
 import { isServerRenderHash } from './isServerRenderResult';
@@ -22,12 +23,13 @@ declare global {
     roots: Root[];
   }
 
-  namespace NodeJS {
-    interface Global {
-      ReactOnRails: ReactOnRailsType;
-      roots: Root[];
-    }
+  namespace globalThis {
+    /* eslint-disable no-var,vars-on-top */
+    var ReactOnRails: ReactOnRailsType;
+    var roots: Root[];
+    /* eslint-enable no-var,vars-on-top */
   }
+
   namespace Turbolinks {
     interface TurbolinksStatic {
       controller?: unknown;
@@ -39,8 +41,6 @@ declare const ReactOnRails: ReactOnRailsType;
 
 const REACT_ON_RAILS_STORE_ATTRIBUTE = 'data-js-react-on-rails-store';
 
-type Context = Window | NodeJS.Global;
-
 function findContext(): Context {
   if (typeof window.ReactOnRails !== 'undefined') {
     return window;

node_package/src/context.ts

@@ -1,8 +1,9 @@
+export type Context = Window | typeof globalThis;
+
 /**
  * Get the context, be it window or global
- * @returns {boolean|Window|*|context}
  */
-export default function context(this: void): Window | NodeJS.Global | void {
+export default function context(this: void): Context | void {
   return ((typeof window !== 'undefined') && window) ||
     ((typeof global !== 'undefined') && global) ||
     this;