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

chore: update module resolution to bundler #1387

Merged
merged 6 commits into from
Jul 8, 2024

Conversation

ryanleecode
Copy link
Contributor

@ryanleecode ryanleecode commented Jul 8, 2024

Fixes: #1386

  • Error boundary had a type export error so had to refactor it to functional component.
  • adds jest/@types/jest to resolve compiler error after node resolution change.
  • Replace usages of /// <reference types="@polkadot/dev-test/globals" /> with import '@polkadot/dev-test/globals.d.ts'; and adds 'import/extensions': 'off' to eslint config. This is required to resolve type issues.

import/extensions is also a useless eslint rule and will be superseded by native node esm resolution. see https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/extensions.md#when-not-to-use-it

@ryanleecode ryanleecode marked this pull request as ready for review July 8, 2024 13:13
@ryanleecode ryanleecode force-pushed the chore/update-module-resolution branch from 2024af7 to aec7cf6 Compare July 8, 2024 13:14
@ryanleecode ryanleecode force-pushed the chore/update-module-resolution branch from aec7cf6 to 0ad380b Compare July 8, 2024 13:14
@ryanleecode ryanleecode requested a review from TarikGul July 8, 2024 13:15
package.json Outdated
"@types/node": "^20.10.5",
"i18next-scanner": "^4.4.0",
"jest": "^29.7.0",
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding jest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

compiler failing with : Cannot find name 'it'. Do you need to install type definitions for a test runner? Try npm i --save-dev @types/jestornpm i --save-dev @types/mocha. after i changed the node resolution

Copy link
Member

Choose a reason for hiding this comment

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

I think this is causing problems in others files with the defined custom global jest types referenced in @polkadot/dev-test.

@TarikGul
Copy link
Member

TarikGul commented Jul 8, 2024

@Tbaut Some eyes on this from you would be nice as well.

@TarikGul TarikGul requested a review from Tbaut July 8, 2024 13:32
@TarikGul
Copy link
Member

TarikGul commented Jul 8, 2024

What types do you specifically need exported?

@ryanleecode
Copy link
Contributor Author

ryanleecode commented Jul 8, 2024

What types do you specifically need exported?

Trying to import @substrate/smoldot-discovery in packages/extension, but i'm getting

Cannot find module '@substrate/smoldot-discovery' or its corresponding type declarations.
  There are types at '/home/ryan/Documents/Repositories/polkadot-js/extension/node_modules/@substrate/smoldot-discovery/dist/index.d.mts', but this result could not be resolved under your current 'moduleResolution' setting. Consider updating to 'node16', 'nodenext', or 'bundler'.ts(2307)

@ryanleecode
Copy link
Contributor Author

ryanleecode commented Jul 8, 2024

I can try scoping the moduleResolution update to just packages/extension

Nah doesnt work

@TarikGul
Copy link
Member

TarikGul commented Jul 8, 2024

What types do you specifically need exported?

Trying to import @substrate/smoldot-discovery in packages/extension, but i'm getting

Cannot find module '@substrate/smoldot-discovery' or its corresponding type declarations.
  There are types at '/home/ryan/Documents/Repositories/polkadot-js/extension/node_modules/@substrate/smoldot-discovery/dist/index.d.mts', but this result could not be resolved under your current 'moduleResolution' setting. Consider updating to 'node16', 'nodenext', or 'bundler'.ts(2307)

I think this is an issue with smoldot-discovery and should be solved at that level no? Instead of completely changing the module resolution for one package in the extension? I would be weary of the potential downstream affects it would have since many wallets and users leverage the extension-* packages but additionally the overhead between @polkadot/dev-ts vs importing Jest. I just want to ensure we don't add any complexity to the already complex pjs repos.

In terms of adding @substrate/smoldot-discovery I am a little out of the loop on that - I think it would be a very large breaking change if it required making webrequests.

@ryanleecode
Copy link
Contributor Author

Should be good now and have no side effects.

@ryanleecode ryanleecode requested a review from TarikGul July 8, 2024 15:13
@TarikGul
Copy link
Member

TarikGul commented Jul 8, 2024

Nice changes, Thank you!

One last thing I would want to be sure of is the potential downstream affects of changing node -> Bundler. Do we know of any issues it could cause?

@ryanleecode
Copy link
Contributor Author

Nice changes, Thank you!

One last thing I would want to be sure of is the potential downstream affects of changing node -> Bundler. Do we know of any issues it could cause?

The code we are emitting is the same so I think its fine; moduleResolution just affects how we resolve imports. module has an impact on what is emitted but we are already on EsNext.

And since we are using rollup, moduleResolution bundler is the way to go.

https://www.totaltypescript.com/concepts/option-module-must-be-set-to-nodenext-when-option-moduleresolution-is-set-to-nodenext#youre-using-the-wrong-moduleresolution

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Thank you!

@TarikGul TarikGul merged commit 727c616 into master Jul 8, 2024
3 checks passed
@TarikGul TarikGul deleted the chore/update-module-resolution branch July 8, 2024 19:15
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jul 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

moduleResolution: Bundler
3 participants