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

Code matching the import regex breaks vite #4027

Closed
6 tasks done
Flakebi opened this issue Jun 29, 2021 · 5 comments · Fixed by #4088
Closed
6 tasks done

Code matching the import regex breaks vite #4027

Flakebi opened this issue Jun 29, 2021 · 5 comments · Fixed by #4088
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@Flakebi
Copy link

Flakebi commented Jun 29, 2021

Describe the bug

Vite contains a heuristic to detect import sources:

// A simple regex to detect import sources. This is only used on
// <script lang="ts"> blocks in vue (setup only) or svelte files, since
// seemingly unused imports are dropped by esbuild when transpiling TS which
// prevents it from crawling further.
// We can't use es-module-lexer because it can't handle TS, and don't want to
// use Acorn because it's slow. Luckily this doesn't have to be bullet proof
// since even missed imports can be caught at runtime, and false positives will
// simply be ignored.
const importsRE =
/\bimport(?!\s+type)(?:[\w*{}\n\r\t, ]+from\s*)?\s*("[^"]+"|'[^']+')/gm

This works most of the time. However, if this regex matches something that is not actually an import, it breaks*.

*: There are some conditions for it to actually break:

  • The vite cache in node_modules/.vite is empty
  • The vite dev server is used (vite, using vite build does not break)

The following svelte code matches the regex and fails to build:

<script lang="ts">
	fetch("import", {
		body: "" });
</script>

Reproduction

<script lang="ts">
	fetch("import", {
		body: "" });
</script>

System Info

Binaries:
    Node: 14.17.1 - /run/current-system/sw/bin/node
    Yarn: 1.22.10 - /run/current-system/sw/bin/yarn
    npm: 6.14.13 - /run/current-system/sw/bin/npm
  npmPackages:
    vite: ^2.3.8 => 2.3.8

Used Package Manager

yarn

Logs

yarn run v1.22.10
warning package.json: No license field
$ vite --debug
  vite:config native esm config loaded in 228ms URL {
  href: 'file:///home/sebi/tmp/viterepro/vite-project/vite.config.js',
  origin: 'null',
  protocol: 'file:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: '/home/sebi/tmp/viterepro/vite-project/vite.config.js',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
} +0ms
  vite:vite-plugin-svelte additional vite config {
  optimizeDeps: {
    exclude: [
      'svelte/animate',
      'svelte/easing',
      'svelte/internal',
      'svelte/motion',
      'svelte/store',
      'svelte/transition',
      'svelte',
      'svelte-hmr/runtime/hot-api-esm.js',
      'svelte-hmr/runtime/proxy-adapter-dom.js',
      'svelte-hmr'
    ]
  },
  resolve: {
    mainFields: [ 'svelte', 'module', 'jsnext:main', 'jsnext' ],
    dedupe: [
      'svelte/animate',
      'svelte/easing',
      'svelte/internal',
      'svelte/motion',
      'svelte/store',
      'svelte/transition',
      'svelte',
      'svelte-hmr/runtime/hot-api-esm.js',
      'svelte-hmr/runtime/proxy-adapter-dom.js',
      'svelte-hmr'
    ]
  }
} +0ms
  vite:vite-plugin-svelte default options for development  {
  extensions: [ '.svelte' ],
  hot: { injectCss: false },
  emitCss: true,
  compilerOptions: { format: 'esm', css: false, dev: true, hydratable: true }
} +16ms
  vite:vite-plugin-svelte resolved options {
  extensions: [ '.svelte' ],
  hot: { injectCss: false },
  emitCss: true,
  compilerOptions: { format: 'esm', css: false, dev: true, hydratable: true },
  preprocess: [
    {
      defaultLanguages: [Object],
      markup: [AsyncFunction: markup],
      script: [AsyncFunction: script],
      style: [AsyncFunction: style]
    },
    { style: [Function: style] }
  ],
  root: '/home/sebi/tmp/viterepro/vite-project',
  isProduction: false,
  isBuild: false,
  isServe: true
} +38ms
  vite:config using resolved config: {
  vite:config   plugins: [
  vite:config     'vite:pre-alias',
  vite:config     'alias',
  vite:config     'vite-plugin-svelte',
  vite:config     'vite:dynamic-import-polyfill',
  vite:config     'vite:resolve',
  vite:config     'vite:html',
  vite:config     'vite:css',
  vite:config     'vite:esbuild',
  vite:config     'vite:json',
  vite:config     'vite:wasm',
  vite:config     'vite:worker',
  vite:config     'vite:asset',
  vite:config     'vite:define',
  vite:config     'vite:css-post',
  vite:config     'vite:client-inject',
  vite:config     'vite:import-analysis'
  vite:config   ],
  vite:config   server: {
  vite:config     fsServe: { root: '/home/sebi/tmp/viterepro/vite-project', strict: false }
  vite:config   },
  vite:config   optimizeDeps: {
  vite:config     exclude: [
  vite:config       'svelte/animate',
  vite:config       'svelte/easing',
  vite:config       'svelte/internal',
  vite:config       'svelte/motion',
  vite:config       'svelte/store',
  vite:config       'svelte/transition',
  vite:config       'svelte',
  vite:config       'svelte-hmr/runtime/hot-api-esm.js',
  vite:config       'svelte-hmr/runtime/proxy-adapter-dom.js',
  vite:config       'svelte-hmr'
  vite:config     ],
  vite:config     esbuildOptions: { keepNames: undefined }
  vite:config   },
  vite:config   resolve: {
  vite:config     dedupe: [
  vite:config       'svelte/animate',
  vite:config       'svelte/easing',
  vite:config       'svelte/internal',
  vite:config       'svelte/motion',
  vite:config       'svelte/store',
  vite:config       'svelte/transition',
  vite:config       'svelte',
  vite:config       'svelte-hmr/runtime/hot-api-esm.js',
  vite:config       'svelte-hmr/runtime/proxy-adapter-dom.js',
  vite:config       'svelte-hmr'
  vite:config     ],
  vite:config     mainFields: [ 'svelte', 'module', 'jsnext:main', 'jsnext' ],
  vite:config     alias: [ [Object] ]
  vite:config   },
  vite:config   configFile: '/home/sebi/tmp/viterepro/vite-project/vite.config.js',
  vite:config   configFileDependencies: [],
  vite:config   inlineConfig: {
  vite:config     root: undefined,
  vite:config     base: undefined,
  vite:config     mode: undefined,
  vite:config     configFile: undefined,
  vite:config     logLevel: undefined,
  vite:config     clearScreen: undefined,
  vite:config     server: { fsServe: [Object] }
  vite:config   },
  vite:config   root: '/home/sebi/tmp/viterepro/vite-project',
  vite:config   base: '/',
  vite:config   publicDir: '/home/sebi/tmp/viterepro/vite-project/public',
  vite:config   cacheDir: '/home/sebi/tmp/viterepro/vite-project/node_modules/.vite',
  vite:config   command: 'serve',
  vite:config   mode: 'development',
  vite:config   isProduction: false,
  vite:config   build: {
  vite:config     target: [ 'es2019', 'edge88', 'firefox78', 'chrome87', 'safari13.1' ],
  vite:config     polyfillDynamicImport: false,
  vite:config     outDir: 'dist',
  vite:config     assetsDir: 'assets',
  vite:config     assetsInlineLimit: 4096,
  vite:config     cssCodeSplit: true,
  vite:config     sourcemap: false,
  vite:config     rollupOptions: {},
  vite:config     commonjsOptions: { include: [Array], extensions: [Array] },
  vite:config     minify: 'terser',
  vite:config     terserOptions: {},
  vite:config     cleanCssOptions: {},
  vite:config     write: true,
  vite:config     emptyOutDir: null,
  vite:config     manifest: false,
  vite:config     lib: false,
  vite:config     ssr: false,
  vite:config     ssrManifest: false,
  vite:config     brotliSize: true,
  vite:config     chunkSizeWarningLimit: 500,
  vite:config     watch: null
  vite:config   },
  vite:config   env: { BASE_URL: '/', MODE: 'development', DEV: true, PROD: false },
  vite:config   assetsInclude: [Function: assetsInclude],
  vite:config   logger: {
  vite:config     hasWarned: false,
  vite:config     info: [Function: info],
  vite:config     warn: [Function: warn],
  vite:config     warnOnce: [Function: warnOnce],
  vite:config     error: [Function: error],
  vite:config     clearScreen: [Function: clearScreen]
  vite:config   },
  vite:config   createResolver: [Function: createResolver]
  vite:config } +72ms
  vite:deps Crawling dependencies using entries:
  vite:deps   /home/sebi/tmp/viterepro/vite-project/index.html +0ms
  vite:vite-plugin-svelte resolveId /home/sebi/tmp/viterepro/vite-project/index.html +159ms
  vite:vite-plugin-svelte resolveId /src/main.ts +13ms
  vite:resolve 2ms   /src/main.ts -> /home/sebi/tmp/viterepro/vite-project/src/main.ts +0ms
  vite:vite-plugin-svelte resolveId {
  id: './App.svelte',
  filename: './App.svelte',
  normalizedFilename: 'App.svelte',
  cssId: './App.svelte?svelte&type=style&lang.css',
  query: [Object: null prototype] {},
  timestamp: 1625003369969,
  ssr: false
} +8ms
  vite:resolve 3ms   ./App.svelte -> /home/sebi/tmp/viterepro/vite-project/src/App.svelte +9ms
 > html:/home/sebi/tmp/viterepro/vite-project/src/App.svelte:6:11: error: Unterminated string literal
    6 │ import ", {
      ╵            ^

error when starting dev server:
Error: Build failed with 1 error:
html:/home/sebi/tmp/viterepro/vite-project/src/App.svelte:6:11: error: Unterminated string literal
    at failureErrorWithLog (/home/sebi/tmp/viterepro/vite-project/node_modules/esbuild/lib/main.js:1449:15)
    at /home/sebi/tmp/viterepro/vite-project/node_modules/esbuild/lib/main.js:1131:28
    at runOnEndCallbacks (/home/sebi/tmp/viterepro/vite-project/node_modules/esbuild/lib/main.js:921:63)
    at buildResponseToResult (/home/sebi/tmp/viterepro/vite-project/node_modules/esbuild/lib/main.js:1129:7)
    at /home/sebi/tmp/viterepro/vite-project/node_modules/esbuild/lib/main.js:1236:14
    at /home/sebi/tmp/viterepro/vite-project/node_modules/esbuild/lib/main.js:609:9
    at handleIncomingPacket (/home/sebi/tmp/viterepro/vite-project/node_modules/esbuild/lib/main.js:706:9)
    at Socket.readFromStdout (/home/sebi/tmp/viterepro/vite-project/node_modules/esbuild/lib/main.js:576:7)
    at Socket.emit (events.js:375:28)
    at addChunk (internal/streams/readable.js:290:12)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Validations

@underfin
Copy link
Member

underfin commented Jul 2, 2021

Close at #4054

@underfin underfin closed this as completed Jul 2, 2021
@Flakebi
Copy link
Author

Flakebi commented Jul 2, 2021

Thanks for the fast fix.
While the example I posted above works correctly now, I think the underlying issue still exists (parsing typescript with a regex is very fragile).

E.g. the following code still crashes:

<script lang="ts">
/*
import this from "non-existant";
*/
</script>

Error message:

yarn run v1.22.10
warning package.json: No license field
$ vite
error when starting dev server:
Error: The following dependencies are imported but could not be resolved:

  non-existant (imported by /home/sebi/tmp/viterepro/vite-project/src/App.svelte)

Are they installed?
    at optimizeDeps (/home/sebi/Downloads/vite/packages/vite/src/node/optimizer/index.ts:170:11)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at runOptimize (/home/sebi/Downloads/vite/packages/vite/src/node/server/index.ts:542:40)
    at Server.httpServer.listen (/home/sebi/Downloads/vite/packages/vite/src/node/server/index.ts:556:9)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@patak-dev patak-dev reopened this Jul 2, 2021
@patak-dev patak-dev added bug p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Jul 2, 2021
@Flakebi
Copy link
Author

Flakebi commented Jul 2, 2021

I’m not sure if that fixes everything :)

<script lang="ts">
//; import this from "non-existant";
</script>

@Flakebi
Copy link
Author

Flakebi commented Jul 3, 2021

Hi,
first of all, thanks for responding and fixing these issues so fast!
I think it’s fine to close this issue if you’re content with the current state.

I’d like to note that regular expressions—which is more or less what regex is—are only able to parse some things, but code is too complex to be parsed correctly with regular expressions (to be exact, context free languages that are not just regular languages cannot be parsed via regular expressions).
See also this post about parsing HTML with regex (another context free, but not regular, language): https://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml-self-contained-tags

In practice, that means, no matter how much the regex to detect imports is tuned, it should always be possible to find an example of valid typescript code where an import is incorrectly detected and the vite dev server subsequently fails to compile the code.

To give an example:

<script lang="ts">
`
import this from "non-existant";
`
</script>

Note that these multiline strings can be nested indefinitely, so trying to count `s doesn’t work out:

<script lang="ts">
`
${`
import a from "b";
`}
`
</script>

A typescript parser is the only tool that will be correct in all cases.

@patak-dev
Copy link
Member

A typescript parser is the only tool that will be correct in all cases.

I'm aware. Thanks for taking the time to write this down and push for Vite to be correct here.

Check out this comment about why this is implemented at this point in this way

// A simple regex to detect import sources. This is only used on

A simple regex to detect import sources. This is only used on <script lang="ts"> blocks in vue (setup only) or svelte files, since seemingly unused imports are dropped by esbuild when transpiling TS which prevents it from crawling further. We can't use es-module-lexer because it can't handle TS, and don't want to use Acorn because it's slow. Luckily this doesn't have to be bullet proof since even missed imports can be caught at runtime, and false positives will simply be ignored.

The changes that we just added thanks to your reports should reduce the number of false positives that look more problematic than false negatives. I don't think we are going to see real cases of the two examples you listed above. If they do, for the first one, we could see if the error we can filter these false positives with some extra checks (maybe checking the path before adding the import?). The second one shouldn't be an issue.

But this may be something that we may be able to better fix as you said removing the regex with a plugin or config later in esbuild. If someone finds a better way, that will be good.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants