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

Cannot pass an npm dependency as an entrypoint to build #6576

Closed
7 tasks done
matthewp opened this issue Jan 20, 2022 · 6 comments · Fixed by #6680
Closed
7 tasks done

Cannot pass an npm dependency as an entrypoint to build #6576

matthewp opened this issue Jan 20, 2022 · 6 comments · Fixed by #6680
Labels
p4-important Violate documented behavior or significantly improves performance (priority) regression The issue only appears after a new release

Comments

@matthewp
Copy link
Contributor

Describe the bug

If you attempt to pass an npm dependency to vite.build's rollupOptions.input you will get an error like this:

Could not resolve entry module (packages/astro/test/fixtures/static-build-frameworks/@astrojs/renderer-lit/client-shim.js).

This is a regression caused here: https://github.com/vitejs/vite/pull/5601/files#diff-aa53520bfd53e6c24220c44494457cc66370fd2bee513c15f9be7eb537a363e7R272-R286

It assumes that all entrypoints are files within the project root and not npm package dependencies.

Reproduction

https://stackblitz.com/edit/vitejs-vite-pcuytf?file=build.mjs

System Info

System:
    OS: macOS 12.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 178.72 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 14.17.0 - ~/.volta/tools/image/node/14.17.0/bin/node
    Yarn: 1.22.10 - ~/.volta/tools/image/yarn/1.22.10/bin/yarn
    npm: 7.11.2 - ~/.volta/tools/image/npm/7.11.2/bin/npm
  Browsers:
    Chrome: 97.0.4692.71
    Firefox: 96.0
    Safari: 15.2

Used Package Manager

npm

Logs

No response

Validations

@Niputi Niputi added the regression The issue only appears after a new release label Jan 20, 2022
@natemoo-re
Copy link
Contributor

For context, Astro's build command needs to be able to pass NPM dependencies as Rollup inputs. This allows us to emit code that isn't referenced directly in src/ files but is generated by our renderers (which are, of course, NPM dependencies configured by the user.)

If this is intentional and there's another approach we should be using, please let us know. This is currently blocking our upgrade to vite@2.8.x.

@patak-dev patak-dev added bug p4-important Violate documented behavior or significantly improves performance (priority) and removed pending triage labels Jan 29, 2022
@sibbng
Copy link
Contributor

sibbng commented Jan 29, 2022

For context, Astro's build command needs to be able to pass NPM dependencies as Rollup inputs. This allows us to emit code that isn't referenced directly in src/ files but is generated by our renderers (which are, of course, NPM dependencies configured by the user.)

If this is intentional and there's another approach we should be using, please let us know. This is currently blocking our upgrade to vite@2.8.x.

I didn't know rollup supports packages as input target. For now you could use require.resolve:
https://stackblitz.com/edit/vitejs-vite-pacpzp?file=build.mjs

In this case we could check the existence of entry point and fallback to original value if it's not exist.

@patak-dev
Copy link
Member

@sibbng the reproduction from @matthewp works fine in rollup, and I think it should also work in Vite. We shouldn't force users to use require.resolve in their configs, and also there is no way to support virtual entry points (that are resolved by the plugin pipeline). See https://rollupjs.org/guide/en/#a-simple-example

@sibbng
Copy link
Contributor

sibbng commented Jan 29, 2022

@patak-dev I'm not saying this doesn't need to be fixed 😀

In this case we could check the existence of entry point and fallback to original value if it's not exist.

@patak-dev
Copy link
Member

I think we should revert and find a very good reason to change the default way rollup input works. Reviewing the issue where we change this, I think we didn't justify the change enough. The linked unocss issue could have been resolved by resolving in the config, no?

@sibbng
Copy link
Contributor

sibbng commented Jan 29, 2022

I think we should revert and find a very good reason to change the default way rollup input works. Reviewing the issue where we change this, I think we didn't justify the change enough. The linked unocss issue could have been resolved by resolving in the config, no?

Ok, I look back the issue in unocss. Error comes from rollup-plugin-dynamic-import-variables. Probably the source of problem is here. I'm ok with the revert.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p4-important Violate documented behavior or significantly improves performance (priority) regression The issue only appears after a new release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants