Skip to content

Conversation

yusinto
Copy link
Contributor

@yusinto yusinto commented Apr 28, 2023

The same cloudflare fix in #107 is applied to vercel in this PR.

@monsagri can you please test this branch in your example app?

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #199363: Fix vercel serialization bug.

@yusinto yusinto changed the base branch from main to yus/sc-199318/fix-edge-sdks-serialization-bug April 28, 2023 08:02
@yusinto yusinto requested review from monsagri and ldhenry April 28, 2023 08:14
"types": ["jest", "node"],
"skipLibCheck": true
"skipLibCheck": true,
"esModuleInterop": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

We need esModuleInterop to be able to import package.json as default import and be consistent with our build instructions/output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the cloudflare and server-node packages don't need esModuleInterop and all the tests passed without it. Did you have an issue without it? Two syntaxes which should work:

import { name, version } from '../package.json';
//or
import * as packageJson from '../package.json';

Can you kindly try these please? Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

f I try and use the package with the named import, I see this error below,

Failed to compile
../js-core/packages/sdk/vercel/dist/esm/src/createPlatformInfo.js
Can't import the named export 'name' (imported as 'name') from default-exporting module (only default export is available)

This error occurred during the build process of the example project and can only be fixed by changing the import.

Using a default import works fine, however then without esModuleInterOp the tests fail as a default import from the package.json isn’t supported

he tests will pass since they use the tsconfig in the package, but have you tried using the built package in a real world setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#109 has an example nextjs app which works without esModuleInterop and works with the vercel sdk with the serialization fix.

Copy link
Contributor

@monsagri monsagri left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on and being so quick with it!

I've made a few changes as discussed over slack and I think we're good to go.

I'll follow up with vercel about updating their SDK to support returning a non parsed config string

@yusinto yusinto mentioned this pull request Apr 28, 2023
Base automatically changed from yus/sc-199318/fix-edge-sdks-serialization-bug to main April 28, 2023 21:05
@yusinto
Copy link
Contributor Author

yusinto commented Apr 28, 2023

Closing this in favour of #114.

@yusinto yusinto closed this Apr 28, 2023
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