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: Enforce no import side effects #12268

Merged
merged 2 commits into from
Feb 26, 2025
Merged

Conversation

spalladino
Copy link
Contributor

@spalladino spalladino commented Feb 25, 2025

Since we enabled verbatimModuleSyntax in yarn project, all imports of the like import { type Foo } from './foo.js' now cause foo.js to be actually imported in runtime. This caused some tests to fail, due to a types-only file in noir-types being imported that jest didn't like (we also need to figure out why jest didn't like it). To prevent this, the type modifier needs to be moved out of the braces. This is what this eslint rule does.

This should allow running tests after compiling with either tsc or swc, so we can go back to using yarn tsc -b -w for local development.

See this post for more info.

Since we enabled `verbatimModuleSyntax` in yarn project, all imports of
the like `import { type Foo } from './foo.js'` now cause `foo.js` to be
actually imported in runtime. To prevent this, the `type` modifier needs
to be moved out of the braces. This is what this eslint rule does.

See [this post](https://typescript-eslint.io/blog/consistent-type-imports-and-exports-why-and-how/#verbatim-module-syntax) for more info.
@@ -34,6 +34,7 @@ module.exports = {
node: true,
},
rules: {
'@typescript-eslint/no-import-type-side-effects': 'error',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only actualy change from this PR. This, and running lint --fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And now the 2nd commit as well

@spalladino spalladino merged commit 83214fc into master Feb 26, 2025
11 checks passed
@spalladino spalladino deleted the palla/no-import-side-effects branch February 26, 2025 01:06
TomAFrench added a commit that referenced this pull request Feb 26, 2025
* master: (92 commits)
  chore: Log prover publisher address on creation (#12267)
  feat: https for bootnode in devnet (#12161)
  feat(avm): support shifts in lookups (#12280)
  feat(docs): Add flamegraph tool to counter contract tutorial (#12202)
  feat(spartan): 192 node 1 tps - additional validator service (#12238)
  feat(avm): class id derivation (#12263)
  docs: Fees doc snippets and code snippets (#12229)
  refactor: proving cost in fee header (#12048)
  fix: prometheus scrapes itself in the cluster (#12277)
  feat: metrics (#12256)
  chore: cleanup stdlib internal imports (#12274)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  fix: Enforce no import side effects (#12268)
  refactor!: note interfaces (#12106)
  yolo undenoise tests
  feat: new transcript functionality to hash elements without including in proof (#12233)
  chore: remove gcloud metrics (#12265)
  ...
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

Successfully merging this pull request may close these issues.

2 participants