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

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

Closed
2 of 3 tasks
arjanvanzutphen opened this issue Aug 22, 2023 · 20 comments
Closed
2 of 3 tasks
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Instant Apps Issues logged by ArcGIS Instant Apps team members. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components-react Issues specific to the @esri/calcite-components-react package. estimate - 3 A day or two of work, likely requires updates to tests. has workaround Issues have a workaround available in the meantime. p - high Issue should be addressed in the current milestone, impacts component or core functionality Professional Services - Services Delivery Issues logged by Professional Services - Services Delivery team members regression Issues that are caused by changes in a release, but were working before that.

Comments

@arjanvanzutphen
Copy link

arjanvanzutphen commented Aug 22, 2023

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/calcite-components
  • @esri/calcite-components-react

Esri team

Professional Services - Services Delivery

@arjanvanzutphen arjanvanzutphen added 0 - new New issues that need assignment. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. needs triage Planning workflow - pending design/dev review. labels Aug 22, 2023
@github-actions github-actions bot added Professional Services - Services Delivery Issues logged by Professional Services - Services Delivery team members p3 - want for upcoming milestone calcite-components-react Issues specific to the @esri/calcite-components-react package. labels Aug 22, 2023
@geospatialem
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

@jcfranco jcfranco added p - high Issue should be addressed in the current milestone, impacts component or core functionality estimate - 3 A day or two of work, likely requires updates to tests. labels Aug 22, 2023
@geospatialem geospatialem added the ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. label Aug 22, 2023
@geospatialem
Copy link
Member

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

value-list-error

@andygup
Copy link
Member

andygup commented Aug 22, 2023

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"
  }
}

@geospatialem geospatialem added this to the 2023 August Priorities milestone Aug 22, 2023
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label Aug 22, 2023
@andygup
Copy link
Member

andygup commented Aug 22, 2023

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:

  • npm i
  • npm run dev
  • View errors in console

@benelan benelan added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Aug 22, 2023
@benelan benelan self-assigned this Aug 22, 2023
@arjanvanzutphen
Copy link
Author

Updated the issue with a repro sample with CCR version 1.6.0

@geospatialem geospatialem added the regression Issues that are caused by changes in a release, but were working before that. label Aug 30, 2023
@benelan
Copy link
Member

benelan commented Aug 31, 2023

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 benelan added the has workaround Issues have a workaround available in the meantime. label Aug 31, 2023
@mpayson
Copy link

mpayson commented Aug 31, 2023

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 added a commit that referenced this issue Aug 31, 2023
…dbox (#7632)

**Related Issue:** #7575

## Summary

CodeSandbox exposes `process`, which makes it look like NodeJS. The only
way to determine it should be treated as the browser is the non-standard
value they use for `process.platform`.

ref: https://nodejs.org/api/process.html#processplatform
@benelan
Copy link
Member

benelan commented Aug 31, 2023

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.

@geospatialem geospatialem removed this from the 2023 August Priorities milestone Sep 1, 2023
@rslibed
Copy link

rslibed commented Sep 6, 2023

This impacts ArcGIS Instant Apps

fyi @kellyhutchins @bcree11 @Csmith246

@rslibed
Copy link

rslibed commented Sep 6, 2023

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

benelan added a commit that referenced this issue Sep 6, 2023
…7671)

**Related Issue:** #7575

## Summary

Vite uses
[`@rollup/plugin-dynamic-import-vars`](https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars)
for resolving dynamic imports that use an expression for the path,
rather than a fixed string. The rollup plugin has a
[limitation](https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#imports-must-start-with--or-)
that requires dynamic imports with expressions to use relative paths.

> All imports must start relative to the importing file. The import
should not start with a variable, an absolute path or a bare import

Changing the module path to a fixed string in the patch script resolves
the errors. It also ensures that only the components being used in the
app are bundled.
@geospatialem geospatialem added ArcGIS Instant Apps Issues logged by ArcGIS Instant Apps team members. 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Sep 6, 2023
@github-actions github-actions bot assigned geospatialem and unassigned benelan Sep 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

Installed and assigned for verification.

@benelan
Copy link
Member

benelan commented Sep 7, 2023

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

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Sep 7, 2023
@geospatialem
Copy link
Member

Verified with 1.8.0

@arjanvanzutphen
Copy link
Author

Updated CCR to 1.8.0 and works perfectly. Thanks!

@andygup
Copy link
Member

andygup commented Sep 7, 2023

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

@geospatialem
Copy link
Member

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

@mpayson
Copy link

mpayson commented Sep 8, 2023

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

@rslibed
Copy link

rslibed commented Sep 8, 2023

Tested and verified for Instant Apps, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Instant Apps Issues logged by ArcGIS Instant Apps team members. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components-react Issues specific to the @esri/calcite-components-react package. estimate - 3 A day or two of work, likely requires updates to tests. has workaround Issues have a workaround available in the meantime. p - high Issue should be addressed in the current milestone, impacts component or core functionality Professional Services - Services Delivery Issues logged by Professional Services - Services Delivery team members regression Issues that are caused by changes in a release, but were working before that.
Projects
None yet
Development

No branches or pull requests

7 participants