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

Deduplicate Common Dependencies between Polykey and Polykey-CLI - and make npm link work #257

Open
CMCDragonkai opened this issue Aug 18, 2024 · 7 comments
Labels
development Standard development

Comments

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Aug 18, 2024

Specification

When developing features that require changes to Polykey and Polykey-CLI, we end up with dealing with the complexity of all the micro-repos we have.

I think perhaps this is a reason why a lot of people use monorepos and just individually package up subdirectories (this is something we might investigate in the future, such as a sort of Polykey-Development repository, because transitive npm link is a question.

One way to avoid having to publish packages every time we cross a package boundary is to use npm link.

Apparently using npm link of Polykey doesn't quite work due to the lack of deduplication of common dependencies. This is because npm by default supports multiple versions of the same dependency. This is due to the way JS modules work, where the module namespace is entirely encapsulated, there's no global namespace conflict. This gives some flexibility to JS projects, but this causes problems when we think there could be an object instance of class A of file 1 being compared to object instance of class A in file 2. Such as the case of asking if x instanceof X which can happen alot in error objects and other sorts of conditional checks.

image

The current workaround right now is to manually symlink the dist directory from Polykey to Polykey-CLI's node_modules/polykey/dist directory. However this isn't very nice.

I believe we can get around this problem by eliminating any common dependency that Polykey is using and just export to Polykey-CLI, so that way Polykey-CLI doesn't depend on errors and logger directly (and they shouldn't really, as these are specific to PK).

I believe at the moment, this corresponds to:

    "@matrixai/errors": "^1.2.0",
    "@matrixai/logger": "^3.1.0",

And looking at where they are being used:

src/errors.ts
1:import type { Class } from '@matrixai/errors';
2:import { AbstractError } from '@matrixai/errors';
src/types.ts
1:import type { LogLevel } from '@matrixai/logger';

src/utils/processors.ts
14:import Logger from '@matrixai/logger';

src/utils/utils.ts
9:import { LogLevel } from '@matrixai/logger';

src/CommandPolykey.ts
8:} from '@matrixai/logger';

src/polykey.ts
31:  } = await import('@matrixai/logger');

tests/identities/claim.test.ts
8:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/identities/discoverQueue.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/identities/allowDisallowPermissions.test.ts
8:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/notifications/outbox/sendReadRemoveClear.test.ts
6:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/notifications/inbox/sendReadRemoveClear.test.ts
6:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/identities/authenticateAuthenticated.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/identities/search.test.ts
8:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/signVerify.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/identities/discoverGet.test.ts
6:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/sessions.test.ts
9:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/keypair.test.ts
1:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/password.test.ts
3:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/encryptDecrypt.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/certchain.test.ts
1:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/public.test.ts
1:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/cert.test.ts
1:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/renew.test.ts
3:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/reset.test.ts
3:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/keys/private.test.ts
1:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/identities/trustUntrustList.test.ts
6:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/unshare.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/delete.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/permissions.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/rename.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/share.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/log.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/pullClone.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/create.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nat/utils.ts
6:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nat/endpointIndependentNAT.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nat/DMZ.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nat/endpointDependentNAT.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/audit/audit.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/scanNode.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nodes/ping.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nodes/connections.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nodes/find.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nodes/claim.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/version.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/nodes/add.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/utils/testAgent.ts
2:import type Logger from '@matrixai/logger';

tests/integration/docker/docker.test.ts
7:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/utils/exec.ts
11:import Logger from '@matrixai/logger';

tests/bootstrap.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/agent/start.test.ts
9:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/agent/unlock.test.ts
3:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/agent/lock.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/agent/status.test.ts
3:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/agent/lockall.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/agent/stop.test.ts
3:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/newDirSecret.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/delete.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/create.test.ts
6:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/list.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/get.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/vaults/list.test.ts
5:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/env.test.ts
6:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/polykey.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/stat.test.ts
3:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/update.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/rename.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

tests/secrets/newDir.test.ts
4:import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';

It should be possible to simply get Polykey to export the logger exports and the error exports.

I can imagine something like polykey/dist/logger and polykey/dist/errors or something similar. We can simply propagate all exports within special files.

With the ESM migration, it would be possible to clean up the way we export things from Polykey, and it may be alot cleaner like polykey/logger and polykey/errors.

Additional context

Tasks

  1. Identify the best way to export logger and errors
  2. Test npm link and confirm that this works without any problems during testing and development
  3. Test transitive npm link, such as between js-quic and Polykey and Polykey-CLI. (By the way, encapsulating modularity is important here to avoid changes that result in developmental cross-cutting concerns that slow down things.)

@tegefaulkes @aryanjassal

@CMCDragonkai CMCDragonkai added the development Standard development label Aug 18, 2024
Copy link

linear bot commented Aug 18, 2024

@CMCDragonkai
Copy link
Member Author

@amydevs relevant to ESM change too.

@CMCDragonkai CMCDragonkai changed the title Deduplicate Common Dependencies between Polykey and Polykey-CLI Deduplicate Common Dependencies between Polykey and Polykey-CLI - and make npm link work Aug 18, 2024
@tegefaulkes
Copy link
Contributor

I've looked into this a little bit and I think our best options are.

  1. Make sure that libraries don't internally check the instance of something for internal logic. js-timer is the main culprit here since it's the main library breaking when we do this kind of linking. Internally it uses an instanceOf check that breaks when linked.
  2. Use npm's more sophisticated linking system by using npm modules. If we have a git submodules repo that wraps all the dependencies for Polykey-CLI then we could work on all dependencies while linked simultaneously if we set up the npm modules. Setting it up is very simple, however when trying this with Polykey-CLI and Polykey at the same time I found that some config with Polykey-CLI broke. This is probably an easy fix we just need to dig into it more.

I think the npm modules is the best approach for this if we can fix the problem with building and configuration.

Copy link
Member Author

If js-timer is always imported from PK as well, then it won't be a problem. I googled it and I cannot find anything about npm modules and this issue is for a quickly fixing the problem, and in general I think PK-CLI should import all relevant things from the PK library, especially if it's passing timer instances into the code.

I actually don't understand where this would cause a problem with the timer instance. If the class file is the same, then it should continue to work.

@tegefaulkes
Copy link
Contributor

It's generally how instanceof works. If I defined two identical classes, they would not be interchangeable from the perspective of instanceof. Previously I thought it was js-timer causing this problem but I went digging and couldn't find the suspect code. I did find PromiseCancelable using instanceof internally to check something.

I still think the best fix is to get npm modules working. It would be the best way to streamline working across repos.

Copy link
Member Author

I don't understand where npm modules is documented. But there shouldn't be any duplicate dependencies we do this fix

@CMCDragonkai
Copy link
Member Author

The usage of instanceof in testing is also affected by ESM migration too due to the new jest vm isolation. See comment MatrixAI/TypeScript-Demo-Lib#32 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development
Development

No branches or pull requests

2 participants