Skip to content
This repository has been archived by the owner on Jan 31, 2023. It is now read-only.

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dominictarr
Copy link
Contributor

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.

@jwerle
Copy link
Member

jwerle commented Aug 12, 2022

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
Copy link
Member

jwerle commented Aug 12, 2022

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 🤷🏼‍♂️

@dominictarr
Copy link
Contributor Author

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?

@heapwolf
Copy link
Member

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.

@jwerle
Copy link
Member

jwerle commented Aug 14, 2022

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

@dominictarr
Copy link
Contributor Author

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.

@jwerle
Copy link
Member

jwerle commented Aug 17, 2022

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

@dominictarr
Copy link
Contributor Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants