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

Unable to resolve circular reference #130

Closed
matthewp opened this issue Dec 31, 2019 · 12 comments
Closed

Unable to resolve circular reference #130

matthewp opened this issue Dec 31, 2019 · 12 comments

Comments

@matthewp
Copy link
Contributor

  • Rollup Plugin Name: @rollup/plugin-commonjs
  • Rollup Plugin Version: 11.0.0
  • Rollup Version: 1.27.14
  • Operating System (or Browser): OSX
  • Node Version: 10.16.3

How Do We Reproduce?

https://gist.github.com/matthewp/59dd4e4f53d1d697168d67a97d9a9867

The above gist contains a minimal example. You can run:

npm run build

To run rollup. Then open smoke.html in any web browser.

Expected Behavior

Expect the code to run without error.

Actual Behavior

An error accessing the joi.object() function. The circular reference is not handled in the commonjs plugin.

@shellscape
Copy link
Collaborator

There are a litany of circular reference issues in the main rollup project. The likelihood that there's a good solution for this is pretty slim.

@matthewp
Copy link
Contributor Author

Yeah I understand that. I wasn't able to recreate using ES modules so I'm fairly certain that the createCommonjsModule code is the culprit here. I can see from looking at it that it's probably very hard to fix.

@matthewp
Copy link
Contributor Author

This case comes down to an ordering issue. joi.js depends on schema.js, but doesn't need it in the module body. schema.js does need joi.js in the module body. If the modules were ordered the opposite it might be ok.

@shellscape shellscape changed the title Commonjs unable to resolve circular reference Unable to resolve circular reference Jan 4, 2020
@Haringat
Copy link
Contributor

@matthewp I'm curious... Would it work if you first imported the schema.js and then imported the main joi entrypoint?

@shellscape
Copy link
Collaborator

From #164:

How Do We Reproduce?

Using @rollup/plugin-commonjs with postcss does not correctly resolve internal files within postcss, for example ./lib/container.js from ./lib/rule.js. The root cause seems to be circular dependencies, caused by mixing import and require statements before Babel transpilation: https://github.com/postcss/postcss/blob/master/lib/container.es6#L609

Here is a complete example: https://glitch.com/edit/#!/mapgrid-rollup-postcss-repro?path=rollup.config.js:1:0

And a repo: https://github.com/mapgrid/rollup-postcss-repro/

Build log

(!) Circular dependencies
node_modules/postcss/lib/container.js -> node_modules/postcss/lib/rule.js -> node_modules/postcss/lib/container.js
...

Runtime error

TypeError: superClass is undefined

Where superClass is the export from ./lib/container.js

@matthewp
Copy link
Contributor Author

@Haringat It might very well, but as I had to switch to webpack for this project I am not able to test and confirm that at this time.

@shellscape
Copy link
Collaborator

Hey folks, this one has gone quite stale. We're going to close for housekeeping but we'd happily review any PRs that pop up to resolve this issue.

@heathermiller

This comment has been minimized.

@shellscape
Copy link
Collaborator

Please don't reply with "me too" or "same" posts. Instead, use the reaction buttons on the original post.

@aaronjensen
Copy link

@shellscape Hopefully this isn't considered a "me too", but... I'm not sure exactly what to do here. I'm trying to use Snowpack+Rollup for the first time and ran into this with zod. The issue is likely the same or very similar to this issue.

Unless I'm missing something, there are still issues with circular dependencies in the rollup commonjs plugin which are known. The only workaround appears to be to change the package being rolled up to work around the circular dependencies (which involves quite a bit of work in 3rd party code in some instances).

Are there plans to revisit this? Would another issue with a different repro help? I highly doubt my single 👍 on a closed issue will make a difference, so I hope you don't mind me chiming in with a little more context here. Thanks!

@shellscape
Copy link
Collaborator

well, there is #658 (searching for the win)

@aaronjensen
Copy link

That's great, thanks. And now there's a reference for folks who only searched issues and not PRs like myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants