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(root): Prevent multi-level imports for @novu/* packages #6596

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

rifont
Copy link
Contributor

@rifont rifont commented Sep 30, 2024

What changed? Why was the change needed?

  • Prevent multi-level imports for @novu/* packages
    • Fix some rules and ignore EE issues and CSS imports

Screenshots

image

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

@@ -3,6 +3,7 @@ import { Logger } from '@nestjs/common';
import sinon from 'sinon';
import { expect } from 'chai';
import { ApiServiceLevelEnum } from '@novu/shared';
// eslint-disable-next-line no-restricted-imports
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to raise an EE PR for these, so they are ignored for now.

*/

/**
* This rule ensures that "multi-level" imports are not used for `@novu/*` packages.
Copy link
Contributor

@djabarovgeorge djabarovgeorge Sep 30, 2024

Choose a reason for hiding this comment

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

I'm wondering if we should approach this from a different direction, specifically by not allowing the @novu domain libraries to have the /src suffix. @novu/**/src For example: @novu/framework/src, @novu/dal/src, @novu/ee/3rd-party/something/src, @novu/novui/src. I believe that, for now, this approach will be more beneficial and won't limit us in potential future implementations.
let me know what you think :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For packages under our control, I'm of the opinion that we should only allow imports from known exports values in the package.json. This benefits us by:

  • keeping module imports clean
  • creating a clear module boundary
  • ensuring that public package consumers can import the same things that we do, allowing us to dogfood

This PR introduces a pattern for exceptions of the rule, namely for multiple entry-point packages. That same pattern can be extended to handle other exceptions we might like to add in the future. I think the benefits of a "by exception" approach outweigh the cons of allowing any multi-level imports right now.

eslint.config.mjs Outdated Show resolved Hide resolved
Copy link

pkg-pr-new bot commented Oct 4, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/novuhq/novu/@novu/node@6596

commit: 7f07c49

Copy link

netlify bot commented Oct 4, 2024

Deploy Preview for novu-stg-vite-dashboard-poc failed. Why did it fail? →

Name Link
🔨 Latest commit 7f07c49
🔍 Latest deploy log https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/66ffb194743fa20008735fed

@rifont rifont merged commit d8348b0 into next Oct 4, 2024
33 of 38 checks passed
@rifont rifont deleted the restrict-multi-level-novu-imports branch October 4, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants