-
Notifications
You must be signed in to change notification settings - Fork 532
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
Comments
The examples should use the files from the
I do not really have an opinion here, but I feel like that
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 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 |
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 Prior to Node 18, all core modules could be imported with or without the I think the advantages to moving the examples to use
Along with the advantages, I think it's worth mentioning that there aren't any drawbacks that I am aware of. |
Thanks for explaining, based on what you say I think we should move to the |
fwiw, I'm not sure if the examples should use 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! |
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. |
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... |
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
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
As a non-maintainer, my opinion is that using |
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. |
You might be able to leverage
There are probably other solutions out there as well, but that was the first thing that came to mind. |
I think that 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. |
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.
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.
Describe the bug
There are a few inconsistencies/bugs in the examples in the repo:
master
branch and those on therelease-1.x
branch.@kubernetes/client-node
, while others use the build fromdist/
.require()
, while others useimport
. It probably makes sense to migrate toimport
. However, the examples that useimport
should probably be renamed to use a.mjs
file extension (otherwise they don't run).node:
scheme.Promise
API directly, while some useasync
/await
. This is not an issue, but it might be worth moving toasync
/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
The text was updated successfully, but these errors were encountered: