-
-
Notifications
You must be signed in to change notification settings - Fork 532
build: separate new comments functionality into a separate path export #1466
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
beaaa8c
to
9bfe1ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Two main questions here and some comments inline.
Make sure it works across all cases
Not sure how well this applies to this specific PR (I think it should work fine), but here's my braindump of all the configs that we should support. I think this will be relevant to test for this PR, but even more so when we do more improvements to the build system:
- does it work in the playground? (npm start). Does it pick up changes automatically?
- does it work in the examples isolated (
npm run dev
a specific example) - almost same as previous point; does it work in stackblitz? (click a stackblitz link in the example). This btw is the reason why we have package / vite / tsconfig etc in the examples, so that we can run the directory of an example directly in stackblitz or similar
- does the website still work in dev mode and picks up changes from source?
- does the website still work in build mode?
I think that should be mostly it 😅
Do we need to enforce specific patterns?
Now we start splitting code more granurarly, should we enforce specific patterns? For example:
- do circular imports make sense or do we want to prevent this? i.e.: can core import comments and v.v.? Or only types, etc?
- If it's allowed; should core import from code in the
comments
directory, or import@blocknote/core/comments
? enforce / block one of both - etc. Basically; besides implementing the technical change are there other things we need to be aware of?
import { BlockNoteView } from "@blocknote/mantine"; | ||
import "@blocknote/mantine/style.css"; | ||
import { useCreateBlockNote } from "@blocknote/react"; | ||
import { MantineProvider, Select } from "@mantine/core"; | ||
import { YDocProvider, useYDoc, useYjsProvider } from "@y-sweet/react"; | ||
import { useMemo, useState } from "react"; | ||
import { HARDCODED_USERS, MyUserType, getRandomColor } from "./userdata"; | ||
import { HARDCODED_USERS, MyUserType, getRandomColor } from "./userdata.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this breaks the next.js docs website where the example is embedded. It either breaks the build / lint / dev environment.
That's why we don't use extensions in the examples. See #956 for original discussion, perhaps you know a better way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it with e5fe1f9
name: "blocknote", | ||
fileName: "blocknote", | ||
formats: ["es", "cjs"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we apply this to all other packages as well? i.e: only es / cjs for all packages.
perhaps we can also streamline this further so we can re-use the vite config across projects, instead of having a duplicated file everywhere
9bfe1ea
to
2169ab2
Compare
2169ab2
to
2682e62
Compare
2682e62
to
fa25266
Compare
I'll look into all these examples & double check, but I think I got all of them working
Yea, there are rules to follow around this. Generally, we should be treating these path exports as though they were different packages, not because it will not work, but because importing a file relatively will cause the code to be included in both bundles. Here is an example: // blocknote/core/src/index.ts
export function XYZ() {
}
// blocknote/core/comments/index.ts
// DO
import { XYZ } from '@blocknote/core'
// DONT: will include the function in this bundle too (defeating the purpose)
import { XYZ } from '../relative-import'
// DO: type imports are fine
import type { ABC } from '../relative-import' As for circular imports, generally this should be avoided as it makes understanding difficult in general. Types are always okay to do strange things like circular imports because generally the types are essentially internal references and typescript is robust enough to resolve pretty gnarly module graphs. JS because there are several bundler implementations should be using the lowest common denominator of not having cycles.
There are ESLint rules for enforcing this for monorepos across package boundaries, but there isn't a hard and fast rule for within a module as there can be use in duplicating the code or allowing things like types to be imported across the package boundary.
We can probably have a document describing the rules around this, but for now I don't think we should introduce more tooling than necessary for this at the moment. Just a discussion on the do's & don'ts is likely sufficient for now |
Thanks! I think it should be possible with either: while we're at it, we might also want to enable https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-relative-packages.md |
This is the start of a new strategy for separating out code packages, using path exports. This organizes the exports for comments related functionality into a new namespace on the
@blocknore/core
package:@blocknote/core/comments