Remove sneaky Node dependencies #233
Conversation
packages/cel/src/internal.ts
Outdated
| export { equals } from "./equals.js"; | ||
| export { createDuration } from "./duration.js"; | ||
| export { getMsgDesc, setEvalContext } from "./eval.js"; | ||
| export type { CelList } from "./list.js"; | ||
| export type { CelMap } from "./map.js"; | ||
| export { Namespace } from "./namespace.js"; | ||
| export { parse } from "./parser.js"; | ||
| export { createRegistryWithWKT } from "./registry.js"; | ||
| export { matchesString } from "./std/logic.js"; | ||
| export type { CelValueTuple } from "./type.js"; | ||
| export type { CelUint } from "./uint.js"; |
There was a problem hiding this comment.
I suspect we'll want to move a lot of these to the primary export, but I did it this way so we could make purposeful decisions about it.
The biggest "gotcha" here might be the parse function since the main export already has a different parse function — we can probably just adapt the tests to use it.
packages/cel/package.json
Outdated
| "./internal": { | ||
| "import": "./dist/esm/internal.js", | ||
| "require": "./dist/cjs/internal.js" |
There was a problem hiding this comment.
This will mean others will also be able to access them as well. Can't we detect the usage of node types using linter for the non test files?
There was a problem hiding this comment.
I think we can split this into 2 PRs. The first is to remove the one node dependency that snuck in today (TextEncoder and TextDecoder). The other one to avoid such things in the future. First one should be pretty straightforward because we do this in other projects as well.
Having the tests (especially unit tests) colocated is very advantageous. IDEs can recognize them, debugging is easier because we are dealing with TS e2e. If we split into a separate package this becomes tougher as the first package has to rebuild and tests depend on the transpiled version. It is not the end of the world but definitely inconvenient. I am not sure if this is worth the tradeoff.
|
@srikrsna-buf Per feedback, pared this down to just the fix without the updates that would have caught the problem. |
No description provided.