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

Inconsistencies in examples #1907

Open
cjihrig opened this issue Oct 4, 2024 · 10 comments
Open

Inconsistencies in examples #1907

cjihrig opened this issue Oct 4, 2024 · 10 comments

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Oct 4, 2024

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

  • There has been some drift between the examples on the master branch and those on the release-1.x branch.
  • Some of the examples use @kubernetes/client-node, while others use the build from dist/.
  • 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).
  • Not really an issue, but the Node core imports can also be updated to use the node: scheme.
  • 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.

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

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
Copy link
Contributor Author

cjihrig commented Oct 6, 2024

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:

  • It is a functional requirement for any of the new core modules I mentioned, even though none of those new modules are used in this repo as far as I know.
  • Removing any ambiguity about what is a core module and what is a userland module for people reading the code.
  • Consistency with the direction Node is moving. If you look at the Node docs, all examples have been migrated to use the node: scheme.

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

@mstruebing
Copy link
Member

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

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

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

mstruebing commented Oct 8, 2024

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

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 added a commit to cjihrig/javascript that referenced this issue Oct 9, 2024
This commit is a first pass at cleaning up the imports in the
project. It moves to the 'node:' scheme for core modules, removes
some unnecessary imports, and removes some unused imports.

Refs: kubernetes-client#1907
cjihrig added a commit to cjihrig/javascript that referenced this issue Oct 10, 2024
This commit is a first pass at cleaning up the imports in the
project. It moves to the 'node:' scheme for core modules, removes
some unnecessary imports, and removes some unused imports.

Refs: kubernetes-client#1907
@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 10, 2024

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

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
Copy link
Contributor Author

cjihrig commented Oct 11, 2024

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

  • Clone the repo and cd into it.
  • Change examples/top.js so that it imports '@kubernetes/client-node' instead of dist.
  • Run node examples/top.js and see it fail because the module is not found.
  • Run npm link && npm link @kubernetes/client-node to set up linking.
  • Run npm ls @kubernetes/client-node and see that it is pointing to the current directory.
  • Run node examples/top.js again and it should work properly.

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

@mstruebing
Copy link
Member

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.

cjihrig added a commit to cjihrig/javascript that referenced this issue Oct 23, 2024
This commit addresses most of the issues raised in kubernetes-client#1907:

- Migrate from require() to import.
- Migrate from Promises API to async/await.
- Use @kubernetes/client-node instead of dist/
- Add examples specific tslint to allow console.

This commit does not sync the examples from the master branch
to the release-1.x branch.
cjihrig added a commit to cjihrig/javascript that referenced this issue Oct 23, 2024
This commit addresses most of the issues raised in kubernetes-client#1907:

- Migrate from require() to import.
- Migrate from Promises API to async/await.
- Use @kubernetes/client-node instead of dist/
- Add examples specific tslint to allow console.

This commit does not sync the examples from the master branch
to the release-1.x branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants