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

Make sure all package have ESM and modules at least, with no cycles #14312

Open
dzearing opened this issue Feb 24, 2023 · 5 comments
Open

Make sure all package have ESM and modules at least, with no cycles #14312

dzearing opened this issue Feb 24, 2023 · 5 comments
Labels
area: packages About how packages are organized and published

Comments

@dzearing
Copy link
Member

dzearing commented Feb 24, 2023

Feature

Please make sure all packages have a module entry that points to an esm entry point for the package.

Right now the local-driver package is missing one:

https://unpkg.com/@fluidframework/local-driver@1.3.5/package.json

It also has a cycle in the import graph that esbuild complains about:

../../node_modules/@fluidframework/local-driver/dist/index.js -> 
../../node_modules/@fluidframework/local-driver/dist/localDocumentService.js -> 
../../node_modules/@fluidframework/local-driver/dist/index.js

Due to this in localDocumentService.ts:

import {
	LocalDeltaStorageService,
	LocalDocumentDeltaConnection,
	LocalDocumentStorageService,
} from ".";

Easy fix here; just change the "." to the appropriate source. Otherwise your modules may not emit in the order the index specifies which can lead to runtime issues.

Might be good to ask how to prevent cycle imports in the future (esbuild will report it, there are also tools like madge. Importing from barrel files within the package can lead to these cycles very easily, good to avoid.

@tylerbutler tylerbutler added the area: packages About how packages are organized and published label Mar 20, 2023
CraigMacomber added a commit that referenced this issue May 17, 2023
## Description

Do two of the fixes suggested by
#14312

Note that this does not solve the general problem that we have no
validation of non-cyclic imports or proper esm exports: it just fixes on
case which was pointed out.
@CraigMacomber
Copy link
Contributor

CraigMacomber commented May 17, 2023

I have fixed the specific cases highlighted here, but not fixed other similar issues nor added tooling to catch these automatically. Thus this issue should stay open to track that work (which is not something I'm working on).

@dzearing
Copy link
Member Author

@CraigMacomber You can get pretty far with just eslint rules. Some ideas:

Disallow from importing from barrel files and .. Only import from source directly and avoid barrel files. This does 3 things:

  • You can avoid implicit cycles
  • Build tools have potentially less work to do, speeding things up
  • It encourages smaller packages (when packages get big, we tend to add index files to manage import complexity. Instead use separate packages. Smaller packages === more versioning options, smaller more well-defined package api surface, and in tooling we can leverage package boundaries to improve speed.

@tylerbutler
Copy link
Member

@dzearing Can you share the eslint rules you're referring to?

@tylerbutler
Copy link
Member

@dzearing i also find the "smaller packages === better" comment interesting. We're struggling under the weight of the hundreds of packages we already have, so it's hard for me to wrap my mind around a model where we produce even more packages.

Copy link
Contributor

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: packages About how packages are organized and published
Projects
None yet
Development

No branches or pull requests

3 participants