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

CRXJS breaks dynamic imports by removing "" around imports #608

Closed
1 of 2 tasks
A-Shleifman opened this issue Dec 12, 2022 · 7 comments · Fixed by #630
Closed
1 of 2 tasks

CRXJS breaks dynamic imports by removing "" around imports #608

A-Shleifman opened this issue Dec 12, 2022 · 7 comments · Fixed by #630

Comments

@A-Shleifman
Copy link
Contributor

Build tool

Vite

Where do you see the problem?

  • In the browser
  • In the terminal

Describe the bug

CRXJS breaks dynamic imports by removing "" around imports

input
import('./test').then(console.log);
output
import(/src/test.ts__t--1670835051549.js).then(console.log);

Imports should be wrapped in quotes, otherwise, they don't work.
These imports work when the project is launched with Vite without CRXJS.

Reproduction

Example provided above

Logs

No response

System Info

System:
    OS: macOS 13.0
    CPU: (10) arm64 Apple M1 Max
    Memory: 12.58 GB / 64.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.17.0 - ~/.nvm/versions/node/v16.17.0/bin/node
    npm: 8.15.0 - ~/.nvm/versions/node/v16.17.0/bin/npm
  Browsers:
    Chrome: 108.0.5359.98
    Safari: 16.1
  npmPackages:
    @crxjs/vite-plugin: ^2.0.0-beta.6 => 2.0.0-beta.7
    vite: ^3.2.4 => 3.2.4

Severity

blocking an upgrade

@lionelhorn
Copy link

lionelhorn commented Dec 23, 2022

Same issue with 2.0.0-beta.9

@A-Shleifman Did you find a workaround / know a previous version that worked?

System:
    OS: Windows 10 10.0.19044
    CPU: (12) x64 Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
    Memory: 38.72 GB / 63.75 GB
  Binaries:
    Node: 16.14.2 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.18 - C:\Program Files\nodejs\yarn.CMD
    npm: 9.1.2 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.19041.1266.0), Chromium (108.0.1462.54)
    Internet Explorer: 11.0.19041.1566
  npmPackages:
    @crxjs/vite-plugin: ^2.0.0-beta.9 => 2.0.0-beta.9
    vite: ^4.0.2 => 4.0.2

@A-Shleifman
Copy link
Contributor Author

const distPath = '/src/utils/index.ts.js';
await import(distPath);

This goes around Vite (which is bad) and it works, BUT only if this file has already been transpiled before. Any changes to this file and descendants will never be loaded even after a restart. This wouldn't work even as a temporary solution.

lionelhorn added a commit to lionelhorn/chrome-extension-tools that referenced this issue Jan 20, 2023
@lionelhorn
Copy link

lionelhorn commented Jan 20, 2023

@A-Shleifman I was able to find the source of the issue.
You can have a look at my fork main...lionelhorn:chrome-extension-tools:86cd570b7a46fcf551df7faf745e7a3a568ddb26

@A-Shleifman
Copy link
Contributor Author

Thank you, @lionelhorn, for looking into this and finding a way to fix the problem. I dug a bit deeper and apparently it's caused by an inconsistency/bug in the lexer. I opened an issue.

Regarding your fork:

  1. It looks like your editor is not respecting the project's prettier config.
  2. I saw another PR which was trying to solve the same (posix) issue a while ago. fixed build vite-plugin on windows (fix #473) #474
  3. The line new URL(_id.slice(PREFIX.length)) makes the build fail

Do you mind if we merge the fix for this issue separately from the posix fix?

@lionelhorn
Copy link

Thanks @A-Shleifman for digging deeper.

  1. It looks like your editor is not respecting the project's prettier config.

Indeed, realized that prettier issue after the fact. I didn't have time to do a proper PR at the time, that's why I only linked my fork for hints to those interesed.

  1. I saw another PR which was trying to solve the same (posix) issue a while ago. fixed build vite-plugin on windows (fix #473) #474

I've seen that PR. That's what I added dirtily on top of 7342ff0

  1. The line new URL(_id.slice(PREFIX.length)) makes the build fail

Didn't get a chance to test the modifications outside of windows.
Build succeeded on windows after the code change but may have broken build for others.

@A-Shleifman
Copy link
Contributor Author

If it helps, this is the error I got when building your fork on my mac:

[!] (plugin bundleClientCode) TypeError: Could not load client//Users/redacted/chrome-extension-tools/packages/vite-plugin/src/client/es/hmr-client-worker.ts (imported by src/node/plugin-background.ts): Invalid URL
TypeError: Could not load client//Users/redacted/chrome-extension-tools/packages/vite-plugin/src/client/es/hmr-client-worker.ts (imported by src/node/plugin-background.ts): Invalid URL
    at new NodeError (node:internal/errors:387:5)
    at URL.onParseError (node:internal/url:564:9)
    at new URL (node:internal/url:640:5)
    at Object.load (/Users/redacted/chrome-extension-tools/packages/vite-plugin/rollup.config.ts:68:21)
    at /Users/redacted/chrome-extension-tools/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/shared/rollup.js:22841:40

@A-Shleifman
Copy link
Contributor Author

@jacksteamdev, can we merge #630?

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

Successfully merging a pull request may close this issue.

2 participants