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

fix: Use commonjs_strict import_type to avoid Functions in generated code #182

Merged
merged 3 commits into from
Aug 5, 2023

Conversation

krispraws
Copy link
Contributor

@krispraws krispraws commented Aug 3, 2023

This is a joint effort between @cprice404 and me to unblock Cloudflare.
Using the websdk in Cloudflare workers gives this error

service core:user:momento-cloudflare-worker-http: Uncaught Error: EvalError: Code generation from strings disallowed for this context

Changes

  • Changes to the generate_protos.sh script to support running npm run build on my mac and fixing permission errors when downloading the plugin
  • Temporarily edit the proto files to remove package declarations from proto files to avoid runtime errors like below, which is similar to this issue / tentative fix
Test suite failed to run

    TypeError: Cannot read properties of undefined (reading 'Never')

       7 | } from '@gomomento/generated-types-webtext/dist/auth_pb';
       8 | import {cacheServiceErrorMapper} from '../errors/cache-service-error-mapper';
    >  9 | import Never = _GenerateApiTokenRequest.Never;
         |                                         ^
      10 | import Expires = _GenerateApiTokenRequest.Expires;
      11 | import {
      12 |   CredentialProvider,

      at Object.<anonymous> (src/internal/auth-client.ts:9:41)
      at Object.<anonymous> (src/auth-client.ts:5:1)
      at Object.<anonymous> (src/index.ts:2:1)
      at Object.<anonymous> (test/integration/integration-setup.ts:13:1)
      at Object.<anonymous> (test/integration/shared/auth-client.test.ts:2:1)
  • Add back the package declaration otherwise we run into
Integration tests for dictionary operations › #dictionaryRemoveField › should remove a Uint8Array field

    Method not found: ScsControl/DeleteCache

      at new E (../../../client-protos/javascript-web/node_modules/grpc-web/index.js:21:1379)
      at Y (../../../client-protos/javascript-web/node_modules/grpc-web/index.js:57:40)
      at qc.<anonymous> (../../../client-protos/javascript-web/node_modules/grpc-web/index.js:55:270)
      at Jb (../../../client-protos/javascript-web/node_modules/grpc-web/index.js:33:182)
      at O (../../../client-protos/javascript-web/node_modules/grpc-web/index.js:32:614)
      at Ac (../../../client-protos/javascript-web/node_modules/grpc-web/index.js:46:68)
      at qc.Object.<anonymous>.n.W (../../../client-protos/javascript-web/node_modules/grpc-web/index.js:44:252)
      at qc.Object.<anonymous>.n.R (../../../client-protos/javascript-web/node_modules/grpc-web/index.js:44:231)
      at XMLHttpRequest.invokeTheCallbackFunction (node_modules/jsdom/lib/jsdom/living/generated/EventHandlerNonNull.js:14:28)
      at XMLHttpRequest.<anonymous> (node_modules/jsdom/lib/jsdom/living/helpers/create-event-accessor.js:35:32)
      at innerInvokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:350:25)
      at invokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:286:3)
      at XMLHttpRequestImpl._dispatch (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:233:9)
      at fireAnEvent (node_modules/jsdom/lib/jsdom/living/helpers/events.js:18:36)
      at readyStateChange (node_modules/jsdom/lib/jsdom/living/xhr/XMLHttpRequest-impl.js:761:3)
      at EventEmitter.<anonymous> (node_modules/jsdom/lib/jsdom/living/xhr/XMLHttpRequest-impl.js:889:5)
      at Request.<anonymous> (node_modules/jsdom/lib/jsdom/living/xhr/xhr-utils.js:385:41)
      at IncomingMessage.<anonymous> (node_modules/jsdom/lib/jsdom/living/helpers/http-request.js:230:42)

Testing

  • Unit and integration tests passed against alpha
  • tested successfully in cloudflare worker after adding a polyfill for XmlHttpRequest using xhrsw

@cprice404
Copy link
Contributor

thanks for battling through this @krispraws ! I added a commit, 8584f70 , with a few minor simplifications to the logic. Let me know what you think!

cprice404
cprice404 previously approved these changes Aug 3, 2023
@krispraws
Copy link
Contributor Author

thanks for battling through this @krispraws ! I added a commit, 8584f70 , with a few minor simplifications to the logic. Let me know what you think!

Your changes look good, @cprice404

@@ -20,6 +21,8 @@
"typescript": "4.4.3"
},
"dependencies": {
"google-protobuf": "3.21.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cprice404 - I had to add these to get the js sdk to build but I am not sure if that was because I was including the dependency as file:.. - do you have any idea if we actually need these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also had to add

"devDependencies": {
    "@types/google-protobuf": "3.15.6",

It got lost when I was committing. Not sure if it is actually needed if I am using a published package.

@cprice404 cprice404 merged commit 2084293 into main Aug 5, 2023
@cprice404 cprice404 deleted the cloudflare_integ branch August 5, 2023 14:50
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

Successfully merging this pull request may close these issues.

2 participants