kubernetes-client / javascript

Javascript client
Apache License 2.0
2.03k stars 519 forks source link

Inconsistencies in examples #1907

Open cjihrig opened 3 weeks ago

cjihrig commented 3 weeks ago

Describe the bug There are a few inconsistencies/bugs in the examples in the repo:

Client Version latest on master and release-1.x branches.

Server Version n/a

To Reproduce View and try to run the various examples.

Expected behavior The examples are consistent and executable.

Example Code n/a

Environment (please complete the following information): n/a

Additional context n/a

mstruebing commented 3 weeks ago

Some of the examples use @kubernetes/client-node, while others use the build from dist/.

The examples should use the files from the dist directory so that you can directly execute them with the current code. Otherwise it would try to use the published npm package.

Some use require(), while others use import. It probably makes sense to migrate to import. However, the examples that use import should probably be renamed to use a .mjs file extension (otherwise they don't run).

I do not really have an opinion here, but I feel like that import is more modern, therefore I would be okay with updating them.

Not really an issue, but the Node core imports can also be updated to use the node: scheme.

Could you explain what the advantage would be? Is there any difference at all? I would guess that if there is a package out there (or in your monorepo/whatever) a package with the same would take precedence whereas with node: this issue doesn't occur?

Some examples use the Promise API directly, while some use async/await. This is not an issue, but it might be worth moving to async/await just to keep the examples simpler.

I vote for using async/await at least in the examples from the release-1.x branch.

cjihrig commented 3 weeks ago

Could you explain what the advantage would be? Is there any difference at all? I would guess that if there is a package out there (or in your monorepo/whatever) a package with the same would take precedence whereas with node: this issue doesn't occur?

There is some background here. Node core modules always take precedent over userland modules. This made it very difficult to ever introduce new core modules because they would either "step on" existing userland modules, or the core team would need to secure the npm module name before using the name for a core module. Enforcing the node: scheme allowed new core modules to be introduced without those problems.

Prior to Node 18, all core modules could be imported with or without the node: scheme. In other words, fs and node:fs work exactly the same. Starting with Node 18, there have been some newly introduced core modules (node:test, node:sqlite, node:sea) that only work with the node: scheme. For example, node:test is a core module, while test would attempt to load something from node_modules/.

I think the advantages to moving the examples to use node: are:

Along with the advantages, I think it's worth mentioning that there aren't any drawbacks that I am aware of.

mstruebing commented 3 weeks ago

Thanks for explaining, based on what you say I think we should move to the node:-scheme. It sounds absolutely reasonable to me. However, it is a non-issue right now so there is no pressure but it would be nice.

brendandburns commented 3 weeks ago

fwiw, I'm not sure if the examples should use dist because I think that is what causes issues to be reported like this:

https://github.com/kubernetes-client/javascript/issues/1844

There's this awkward balance of "examples work when I check out the repo" vs "examples work when I try to create something myself in my own repo"

Most developers are experienced enough to understand the differences, but I think it trips up some new developers.

Anyway, @cjihrig we'd welcome any clean ups you want to send as PRs.

Thanks!

mstruebing commented 3 weeks ago

Okay I get your point, we could also assume that someone who works on this project is experienced enough to run the examples with the correct code. Anything that confuses users is bad and user experience > dev experience in my point of view. It was just the reason back then why it was switched.

brendandburns commented 3 weeks ago

One thing we did in the Java repo was to have two different examples dirs, like "examples-HEAD" and "examples-release" but that can be confusing too...

cjihrig commented 2 weeks ago

As a non-maintainer, my opinion is that using @kubernetes/client-node in the examples makes the most sense. I think there are likely to be more users that want to use the module without even bothering to clone the repo. For example, if I wanted to get started with informers, I might find the examples via the GitHub UI and copy+paste into my own file to get started and then run npm install. Personally, I wouldn't clone the repo unless I was planning to contribute back.

brendandburns commented 2 weeks ago

I'm generally in agreement with @cjihrig here. The only challenge then is how we keep the examples working via CI/CD as we move towards breaking changes at HEAD.

It's probably only a small issue though, so I agree with the preference for a reference to the released library.

cjihrig commented 2 weeks ago

You might be able to leverage npm link in the CI. I haven't verified it, but the following works for me locally:

There are probably other solutions out there as well, but that was the first thing that came to mind.

mstruebing commented 2 weeks ago

I think that npm link is a very solid solution in this situation.

I don't really like the multiple directory examples solution from the java repo. The only other way I can think of right now would be to manipulate the test files in CI i.e. find ... | xargs 'sed/<BAD_IMPORT>/<GOOD_IMPORT>' but that's error prone and hacky in my opinion.