Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
286 stars 76 forks source link

auto-define.js issue prevents Calcite Components React from loading #7575

Closed arjanvanzutphen closed 1 year ago

arjanvanzutphen commented 1 year ago

Check existing issues

Actual Behavior

Many, if not all, components don't load due to an auto-define.js error log. I noticed that also components are mentioned in the error logs that I don't use in my application image

Expected Behavior

The application loads as expected

Reproduction Sample

https://github.com/arjanvanzutphen/ccr-auto-define-issue

Reproduction Steps

  1. Clone the repo
  2. Run npm install
  3. Run npm run dev
  4. Open the page. Most likely: http://localhost:5173
  5. See errors in Devtools' console

Reproduction Version

1.6.0. in sample

Relevant Info

1.6.1 shows the same behavior and is not showing the components as they should

Regression?

1.5.1

Priority impact

p3 - want for upcoming milestone

Impact

The app doesn't get loaded. Version 1.6.0 is in this way unusable

Calcite package

Esri team

Professional Services - Services Delivery

geospatialem commented 1 year ago

CodeSandbox can sometimes takes a few days to catch up to the latest, but if you open the template and select the "Upgrade to latest" button in the "Dependencies" it will update to 1.6.1. image

Or if its easier to setup a use case, here's the sample with 1.6.1 setup.

@arjanvanzutphen WRT the above, could you add a sample with either the template upgraded to latest, or with the above link?

jcfranco commented 1 year ago

Looks like the isBrowser check we set up is failing in this case. window, document, global and process are available on the browser preview. cc @benelan

jcfranco commented 1 year ago

Maybe we can add some additional checks on the process object since we can't rely on these not being available because of bundlers. We can also look at using https://www.npmjs.com/package/browser-or-node or beefing up our checks based on that.

geospatialem commented 1 year ago

This also surfaced with the Maps SDK for JS team during initialization cc: @andygup.

value-list-error
andygup commented 1 year ago

Thanks @geospatialem, I was able to repro using:

  "dependencies": {
    "@arcgis/core": "^4.27.6",
    "@esri/calcite-components-react": "^1.6.1",
    "react": "^18.2.0",
    "react-dom": "^18.2.0"
  },
  "devDependencies": {
    "@vitejs/plugin-react": "^4.0.3",
    "vite": "^4.4.5"
  }
}
andygup commented 1 year ago

Here's a repro app: https://devtopia.esri.com/andy4683/arcgis-calcite-vite-react. It should be using the latest version of all the deps.

Steps:

arjanvanzutphen commented 1 year ago

Updated the issue with a repro sample with CCR version 1.6.0

benelan commented 1 year ago

Apparently vite requires a plugin to handle dynamic imports the way webpack does: https://www.npmjs.com/package/vite-plugin-dynamic-import

Using the dynamic import plugin with the following vite config resolved the issue for me:

import { defineConfig } from "vite";
import react from "@vitejs/plugin-react";
import dynamicImport from "vite-plugin-dynamic-import";
import { resolve } from "path";

// https://vitejs.dev/config/
export default defineConfig({
  resolve: {
    alias: {
      "@esri/calcite-components": resolve(
        "node_modules",
        "@esri",
        "calcite-components",
      ),
    },
  },
  plugins: [
    react(),
    dynamicImport({
      filter(id) {
        return id.includes("/node_modules/@esri/calcite-components");
      },
    }),
  ],
});

Let me know if that doesn't work for anyone.

For reference, we needed to use a dynamic import to make sure web components aren't defined on the server when using SSR frameworks. I'll switch our CRA example that I use for testing to a Vite one since that's the industry standard nowadays.

mpayson commented 1 year ago

Apparently vite requires a plugin to handle dynamic imports the way webpack does:

Is there a way to do this without requiring a plugin, maybe by optionally disabling auto-import? This is not an official Vite plugin and it does not have a license file. I also worry that the auto-import is kind of magical, and could lead to hard-to-diagnose bugs (eg with hot reloading, lazy loading, test runners...) or broken environments with other bundlers (Parcel...)

It also looks like tree shaking is not supported with the React output target, which may mean auto-import will import all react wrappers and all stencil components, which is why we are seeing errors from unused modules. Currently, I think we can control which stencil components are imported to manage bundle size

To me the auto-import feature feels like a major change with some unknowns, and it may be worth making it opt-in to see the impact on different apps first, and maybe delaying a full re-release until a major / breaking calcite version

benelan commented 1 year ago

I'll see if I can solve this in the vite config without using that plugin.

It also looks like tree shaking is not supported with the React output target, which may mean auto-import will import all react wrappers and all stencil components, which is why we are seeing errors from unused modules.

When I was testing with your repro sample I only saw errors for all components in the dev server. When building and serving there was only one error for the component being used in the app.

To me the auto-import feature feels like a major change with some unknowns, and it may be worth making it opt-in to see the impact on different apps first, and maybe delaying a full re-release until a major / breaking calcite version

I can look into making it opt in, but it would be adding even more magic because we are already patching Stencil's build to get it to work as is. The auto import already went out in 1.5.0 so unfortunately we can't delay it, and removing it would be a breaking change.

rslibed commented 1 year ago

This impacts ArcGIS Instant Apps

fyi @kellyhutchins @bcree11 @Csmith246

rslibed commented 1 year ago

Apparently vite requires a plugin to handle dynamic imports the way webpack does: https://www.npmjs.com/package/vite-plugin-dynamic-import

Using the dynamic import plugin with the following vite config resolved the issue for me:

import { defineConfig } from "vite";
import react from "@vitejs/plugin-react";
import dynamicImport from "vite-plugin-dynamic-import";
import { resolve } from "path";

// https://vitejs.dev/config/
export default defineConfig({
  resolve: {
    alias: {
      "@esri/calcite-components": resolve(
        "node_modules",
        "@esri",
        "calcite-components",
      ),
    },
  },
  plugins: [
    react(),
    dynamicImport({
      filter(id) {
        return id.includes("/node_modules/@esri/calcite-components");
      },
    }),
  ],
});

Let me know if that doesn't work for anyone.

For reference, we needed to use a dynamic import to make sure web components aren't defined on the server when using SSR frameworks. I'll switch our CRA example that I use for testing to a Vite one since that's the industry standard nowadays.

@benelan Tried out the workaround but it doesn't appear to resolve the issue in my app. App is using @esri/calcite-components@1.7.0

Got it to work

github-actions[bot] commented 1 year ago

Installed and assigned for verification.

benelan commented 1 year ago

You should no longer need the vite plugin after updating to the v1.8.0 release that went out a few hours ago!

geospatialem commented 1 year ago

Verified with 1.8.0

arjanvanzutphen commented 1 year ago

Updated CCR to 1.8.0 and works perfectly. Thanks!

andygup commented 1 year ago

Verified with JS SDK 4.27.6 and @arcgis/core@4.28.0-next.20230907.

geospatialem commented 1 year ago

Great to hear, thank you both @arjanvanzutphen and @andygup! 🎉

mpayson commented 1 year ago

Working for us too -- for Vite dev server, Vite production builds, and Vitest -- thank you!!

rslibed commented 1 year ago

Tested and verified for Instant Apps, thanks!