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: build target for vercel will throw error #699

Merged
merged 5 commits into from
May 6, 2024

Conversation

PerfectPan
Copy link
Contributor

@PerfectPan PerfectPan commented May 5, 2024

Solve #700

The bug was introduced in #682, the reason is that the logic of https://github.com/dai-shi/waku/pull/682/files#diff-6de2cc9d9f6423e3f4faa0cb1a6200112f1bc057af0db1a9362fe6117c51d18aR79-R82 was adjusted.

I think the logic now assmues the entry file is endsWith .jsx while while the dynamic import in serve-vercel.js is endsWith .js

CleanShot 2024-05-05 at 17 02 38@2x

CleanShot 2024-05-05 at 17 03 30@2x

I am not so sure it is a correct way to remove .js extension, but I added the dry run testing, maybe it can help.

Copy link

vercel bot commented May 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview May 6, 2024 0:06am

Copy link

codesandbox-ci bot commented May 5, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@@ -38,7 +38,7 @@ export function rscServePlugin(opts: {
config(viteConfig) {
// FIXME This seems too hacky (The use of viteConfig.root, '.', path.resolve and resolveFileName)
const entriesFile = resolveFileName(
path.resolve(viteConfig.root || '.', opts.srcDir, SRC_ENTRIES + '.js'),
path.resolve(viteConfig.root || '.', opts.srcDir, SRC_ENTRIES),
Copy link
Owner

Choose a reason for hiding this comment

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

Can you try this?

Suggested change
path.resolve(viteConfig.root || '.', opts.srcDir, SRC_ENTRIES),
path.resolve(viteConfig.root || '.', opts.srcDir, SRC_ENTRIES + '.jsx'),

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 tried, it seems ok as well.

@PerfectPan
Copy link
Contributor Author

All the windows tests are failed. Maybe there are another issues?

@dai-shi
Copy link
Owner

dai-shi commented May 5, 2024

probably, wrap with normalizePath.

@PerfectPan
Copy link
Contributor Author

probably, wrap with normalizePath.

Ha, you are right

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Great it works. Thanks for your contribution!
I really appreciate it. (As noted as "FIXME", I'm not super comfortable with the current solution. Hopefully, we will revisit it.)

@PerfectPan
Copy link
Contributor Author

I see the failed test stuck on actions/setup-node downloading, can you rerun the test?

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't notice before. Can you try this please?

Comment on lines 41 to 42
const entriesFile = resolveFileName(
path.resolve(viteConfig.root || '.', opts.srcDir, SRC_ENTRIES + '.js'),
normalizePath(
Copy link
Owner

Choose a reason for hiding this comment

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

Wait, on second look, I think normalizePath should be outside of resolveFileName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I think you are right. resolveFileName will use node:fs to check file exists.

import { rm } from 'node:fs/promises';
import { expect } from '@playwright/test';

const cwd = fileURLToPath(new URL('../examples/01_template', import.meta.url));
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add test with 20_minimal too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the adjustment.

@dai-shi dai-shi merged commit 4606f1d into dai-shi:main May 6, 2024
28 checks passed
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.

[BUG]: build target for vercel or other platform(not SSG situation) will throw error
2 participants