socket-api icon indicating copy to clipboard operation
socket-api copied to clipboard

refactor using browser field to make tests runnable in node.js

Open dominictarr opened this issue 3 years ago • 8 comments

this makes it possible to import @socketsupply/io from inside of node. and more importantly, enables the tests to be run in node. This is necessary to confirm that the behaviour that we are testing is actually what node does. Also, these tests define the node features that we support. (because I'm sure that there are some we will not be able to)

esbuild supports the browserify browser field, which specifies a different file to use while bundling. so when loading "os" it loads "./sdk/os" but if you import "@socketsupply/io/os" from inside of node it will actually just return the "os" module.

There really isn't very much test coverage yet. Probably we will be able to just copy quite a few tests from node.js.

dominictarr avatar Aug 12 '22 09:08 dominictarr

why move all of this into a sdk folder? if we care so much about node compat, why not create a node folder and put the node.js code there?

jwerle avatar Aug 12 '22 12:08 jwerle

I guess maybe a src directory could work but the node stuff feels out of place when this module is intended to work in one type of of environment 🤷🏼‍♂️

jwerle avatar Aug 12 '22 12:08 jwerle

I put everything into the SDK because of how the browser field works. It allows us to override the file path when bundling. We can't put the node stuff in a folder, because then import "@socketsupply/fs" wouldn't work. it would have to be import "@socketsupply/node/io" which would make it not drop in compatible. With require it's possible to put the require in an if branch and control what is loaded but you can't do that with import, so we are lucky to have this capability.

src would be fine, but to me that indicates source as in code that is compiled to something. doesn't matter what the folder is called though, but there needs to be one, or at least a file name prefix?

dominictarr avatar Aug 14 '22 07:08 dominictarr

I guess maybe a src directory could work but the node stuff feels out of place when this module is intended to work in one type of of environment 🤷🏼‍♂️

I like using node as a reference point for something that works. It was really helpful to write a simple dgram server in node and then just check that the exact same code works in socket-sdk. But for sure I'd rather see a src dir than a dir with an arbitrary name like sdk.

heapwolf avatar Aug 14 '22 16:08 heapwolf

I guess I am just not totally sold on why this repository needs to have anything to do with node, or even if it does, supports it in any first class manner. I get the need for verifying API design and function IO for parity, but why would anyone import fs from this and expect to get a nodejs FS when you can just import `node:FS| directly. This module is for the SDK webview environment

jwerle avatar Aug 14 '22 16:08 jwerle

nodejs FS when you can just import `node:FS| directly. This module is for the SDK webview environment

yes but what we are doing here is trying to make development easy again. we want everything to be the same on all the platforms.

most importantly here, we are trying to build a platform for other people to build on. other people really need a stable foundation. On one level it's more important that it's stable than it's like something. But if it is compatible with a subset of node then it's easier for people who maybe already have code they want to migrate, or, already have experience they don't have to re learn. Also it's easier for us because we already have a reference implentation of what it's meant to be, so we don't have to spend time deciding what the interfaces are like, just make them the same.

It's already sufficiently the same that I thought the point was to make node compatible, using the same module and function names, so I'm kinda surprised to be arguing about this, and I think our users would see the same thing.

dominictarr avatar Aug 17 '22 09:08 dominictarr

nodejs FS when you can just import `node:FS| directly. This module is for the SDK webview environment

yes but what we are doing here is trying to make development easy again. we want everything to be the same on all the platforms.

most importantly here, we are trying to build a platform for other people to build on. other people really need a stable foundation. On one level it's more important that it's stable than it's like something. But if it is compatible with a subset of node then it's easier for people who maybe already have code they want to migrate, or, already have experience they don't have to re learn. Also it's easier for us because we already have a reference implentation of what it's meant to be, so we don't have to spend time deciding what the interfaces are like, just make them the same.

It's already sufficiently the same that I thought the point was to make node compatible, using the same module and function names, so I'm kinda surprised to be arguing about this, and I think our users would see the same thing.

we were striving for API compat, not an isomorphic module that can be used with the node runtime. However, if we really want that experience, I think we should organize the code here differently

jwerle avatar Aug 17 '22 15:08 jwerle

we were striving for API compat, not an isomorphic module that can be used with the node runtime.

an isomorphic module that can be used with the node runtime. is what we should make. because it's not just about apps, those apps are built with modules, and we need the modules to work. as a js developer I expect to see a module on npm, install it, and it works in my app. if those modules work in socketsupply it's a lot more compelling as a platform.

We don't even need to change very much. just shift around a few files etc, so that it's runnable in node, AND we also get the benefit of being about to run the tests in node and thus verify that we do actually have API compat. Imo it's worth it just for that

dominictarr avatar Aug 20 '22 21:08 dominictarr