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

Circular dependencies in @substrate/smoldot-light #2613

Closed
jacogr opened this issue Aug 11, 2022 · 4 comments
Closed

Circular dependencies in @substrate/smoldot-light #2613

jacogr opened this issue Aug 11, 2022 · 4 comments

Comments

@jacogr
Copy link

jacogr commented Aug 11, 2022

When including the package in a rollup build, it has some complaints. (Generally, apart from madge the rollup bundler is actually quite good at sniffing out these things). The full output from rollup is as follows -

packages/api/build/bundle.js → packages/api/build/bundle-polkadot-api.js...
(!) `this` has been rewritten to `undefined`
https://rollupjs.org/guide/en/#error-this-is-undefined
node_modules/@substrate/smoldot-light/dist/mjs/client.js
2: // Copyright (C) 2019-2022  Parity Technologies (UK) Ltd.
3: // SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
4: var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
                    ^
5:     function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
6:     return new (P || (P = Promise))(function (resolve, reject) {
...and 1 other occurrence
node_modules/@substrate/smoldot-light/dist/mjs/instance/instance.js
2: // Copyright (C) 2019-2022  Parity Technologies (UK) Ltd.
3: // SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
4: var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
                    ^
5:     function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
6:     return new (P || (P = Promise))(function (resolve, reject) {
...and 1 other occurrence
node_modules/@substrate/smoldot-light/dist/mjs/instance/raw-instance.js
2: // Copyright (C) 2019-2022  Parity Technologies (UK) Ltd.
3: // SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
4: var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
                    ^
5:     function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
6:     return new (P || (P = Promise))(function (resolve, reject) {
...and 1 other occurrence
(!) Circular dependency
node_modules/@substrate/smoldot-light/dist/mjs/client.js -> node_modules/@substrate/smoldot-light/dist/mjs/instance/instance.js -> node_modules/@substrate/smoldot-light/dist/mjs/client.js

The this issue above seems to be caused by using await in the code and the build step, have not dug to see "why".

For the last issue in the log, the circular dep from client <-> instance, a solution could be -

  • split all the classes extending Error into errors.ts
  • import the specific error we need from errors.ts in instance.ts
  • import/re-export the errors as required in client.ts

A suggestion, the team here would have their own ideas and know best. I just want to shine light on the above log.

@wirednkod
Copy link
Contributor

Thank you @jacogr for this. I already started looking into it, so I hopefully will have a solution on these warnings soon

@tomaka
Copy link
Contributor

tomaka commented Aug 12, 2022

For the __awaiter-related issue, I don't know much what we can do. This little var __awaiter block is automatically generated by TypeScript because we're targeting es6. None of the modules use await at the root level, only within function bodies.

@wirednkod wirednkod removed their assignment Aug 12, 2022
@jacogr
Copy link
Author

jacogr commented Aug 12, 2022

Looking at the Rollup error link, i.e. https://rollupjs.org/guide/en/#error-this-is-undefined - I would most-probably place that squarely on the rollup config where this is used to ensure it specifies a sane context. (At least in my bundle configs this is something I'm going to be doing - this is not the only library I found with something like)

@tomaka
Copy link
Contributor

tomaka commented Aug 14, 2022

Closing, since the circular dependency has been addressed.

@tomaka tomaka closed this as completed Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants