Open CMCDragonkai opened 3 months ago
@amydevs relevant to ESM change too.
I've looked into this a little bit and I think our best options are.
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.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.
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.
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.
I don't understand where npm modules is documented. But there shouldn't be any duplicate dependencies we do this fix
The usage of instanceof
in testing is also affected by ESM migration too due to the new jest vm isolation. See comment https://github.com/MatrixAI/TypeScript-Demo-Lib/issues/32#issuecomment-2306946120
Specification
When developing features that require changes to
Polykey
andPolykey-CLI
, we end up with dealing with the complexity of all the micro-repos we have.One way to avoid having to publish packages every time we cross a package boundary is to use
npm link
.Apparently using
npm link
ofPolykey
doesn't quite work due to the lack of deduplication of common dependencies. This is becausenpm
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 ifx instanceof X
which can happen alot in error objects and other sorts of conditional checks.The current workaround right now is to manually symlink the
dist
directory fromPolykey
toPolykey-CLI
'snode_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 toPolykey-CLI
, so that wayPolykey-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:
And looking at where they are being used:
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
andpolykey/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 likepolykey/logger
andpolykey/errors
.Additional context
.npmrc
files inside NPM packages can no longer set theprefix
option, and it needs to be removed from all of our repos. I've been doing this as I see them. However the prefix option should be set in~/.npmrc
so that way npm linking can work. It should be set topreifx=~/.npm
.Tasks
logger
anderrors
npm link
and confirm that this works without any problems during testing and developmentnpm link
, such as betweenjs-quic
andPolykey
andPolykey-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