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

fix: revert #5601 #6025, don't resolve rollupOptions.input #6680

Merged
merged 1 commit into from
Jan 30, 2022

Conversation

patak-dev
Copy link
Member

Description

fix #6576

In [email protected] we merged #5601 resolving the paths in rollupOptions.input. This was done to fix path issues in Windows that are not clear enough from the issue. See unocss/unocss#88 (the issue there no longer seems relevant to unocss as the playground no longer uses rollupOptions.input in its vite config)

The added resolve was the basic one we use for lib mode entry, and doesn't take into account virtual paths or paths to dependencies that are valid inputs in rollup. See for example https://rollupjs.org/guide/en/#a-simple-example

This PR adds a basic test case in one of the playgrounds, and reverts the change. We should do this in a minor in case someone in the ecosystem started to rely on the new resolved config.

Would you help us review and test this PR @sibbng @IanVS @ydcjeff @matthewp?


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added this to the 2.8 milestone Jan 29, 2022
@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Jan 29, 2022
@sibbng
Copy link
Contributor

sibbng commented Jan 29, 2022

We don't need to revert this changes: Source of problem is just unhandled use case and workaround is available: #6576 (comment)

Copy link
Contributor

@IanVS IanVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads-up, @patak-dev. This should not be a problem for storybook-builder-vite.

@patak-dev patak-dev merged commit 2a9da2e into main Jan 30, 2022
@patak-dev patak-dev deleted the fix/revert-5601-6025 branch January 30, 2022 16:46
@matthewp
Copy link
Contributor

Looking at the code I think this will fix our issue. Thanks!

@ckvv
Copy link

ckvv commented Feb 14, 2022

@patak-dev
This update will often trigger the following error?

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined

const resolve = (p: string) => path.resolve(config.root, p)
const input = libOptions

If it's because my configuration file is not standardized, I think it should give a more friendly prompt?

Upgrading https://github.com/ckpack/v-ui-template dependency vite to 2.8.1 will trigger the bug

@patak-dev
Copy link
Member Author

@ckvv would you open a new issue so we can properly track this? And PR welcome with the warning you are thinking of so we can discuss it looking at the change that would help others to understand the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot pass an npm dependency as an entrypoint to build
5 participants