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

5.1.x Regression - dynamic named import in dev mode not working as expected #16106

Closed
7 tasks done
n3dst4 opened this issue Mar 6, 2024 · 2 comments · Fixed by #16113
Closed
7 tasks done

5.1.x Regression - dynamic named import in dev mode not working as expected #16106

n3dst4 opened this issue Mar 6, 2024 · 2 comments · Fixed by #16113
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release

Comments

@n3dst4
Copy link

n3dst4 commented Mar 6, 2024

Describe the bug

I am importing the library xss using a dynamic, named import, like this:

    const { FilterXSS } = await import("xss");
    const xss = new FilterXSS({});

I expect this to import the FilterXSS value from the xss library and call it as a constructor.

  • This works correctly in VIte 5.0.x in dev mode and production/preview mode.
  • It also works correctly in Vite 5.1 in production/preview mode.
  • However in Vite 5.1.x, in dev mode, FilterXSS is undefined.

Also worth noting:

  • Static default import import xss from "xss"; works fine in all situations.
  • Static named import import { FilterXSS } from "xss"; works fine in all situations.
  • Dynamic default import (await import("xss")).default also works fine in all situation.

Reproduction

https://github.com/n3dst4/vite-xss-import-bug-repro

Steps to reproduce

Clone the repo https://github.com/n3dst4/vite-xss-import-bug-repro

Install deps

pnpm install

To run in dev mode (to see the broken behavior):

pnpm dev

To run in production mode (wherein the bug does not manifest):

pnpm preview

System Info

System:                                                           
    OS: Linux 5.15 Ubuntu 22.04.3 LTS 22.04.3 LTS (Jammy Jellyfish) 
    CPU: (24) x64 AMD Ryzen 9 5900X 12-Core Processor               
    Memory: 25.44 GB / 31.31 GB                                     
    Container: Yes                                                  
    Shell: 5.8.1 - /usr/bin/zsh                                     
  Binaries:                                                         
    Node: 20.11.1 - ~/.nvm/versions/node/v20.11.1/bin/node          
    Yarn: 1.22.19 - ~/.nvm/versions/node/v20.11.1/bin/yarn          
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.1/bin/npm             
    pnpm: 8.15.0 - ~/.nvm/versions/node/v20.11.1/bin/pnpm           
  Browsers:                                                         
    Chromium: 122.0.6261.94                                         
  npmPackages:                                                      
    @vitejs/plugin-react-swc: ^3.5.0 => 3.6.0                       
    vite: ^5.1.5 => 5.1.5

Used Package Manager

pnpm

Logs

No response

Validations

@n3dst4 n3dst4 changed the title 5.1 Regression - dynamic named import in dev mode not working as expected 5.1.x Regression - dynamic named import in dev mode not working as expected Mar 6, 2024
@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Mar 7, 2024

Thanks for the reproduction with comparisons. It looks like this is a similar issue as #15851 and a regression from #15619

Though it's a clear inconsistency between dev and build, I think the general safer recommendation is to use default import for originally CJS dependency (which is internally transpiled into ESM by Vite).

The code now checks typeof cjs === "object", but this is probably failing since xss's exports object is a function:

https://github.com/leizongmin/js-xss/blob/66df7c48d5280de27b787169165afc4ea850cda4/lib/index.js#L23-L25

exports = module.exports = filterXSS;
exports.filterXSS = filterXSS;
exports.FilterXSS = FilterXSS;

@hi-ogawa hi-ogawa added p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release and removed pending triage labels Mar 7, 2024
@XiSenao
Copy link
Collaborator

XiSenao commented Mar 7, 2024

Thank you for the explanation.

The current issue and #15851 are different. I may not consider #15851 to be a vite problem, because I don't think it's reasonable to reference properties on the prototype chain in namespace imports and this may be a issue with rollup. The current issue is due to the lack of extension of function properties, which was indeed overlooked in PR.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 23, 2024
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) regression The issue only appears after a new release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants