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

feat: Test bundling packages which provide environment depended code parts #484

Merged
merged 6 commits into from
Feb 14, 2024

Conversation

aheissenberger
Copy link
Contributor

As requested #481 this is a simpler Version which does not use screen shots to test results.

Choosing the right code based on the export section of the react-textarea-autosize module based on the backend server deployment target ('node' | 'webworker') and the client browser. No browser only code should be bundled with server only code.

You can simple test this fixture by setting isNodeCompatible to false

Copy link

vercel bot commented Feb 13, 2024

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

1 Ignored Deployment
Name Status Preview Updated (UTC)
waku ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 0:22am

Copy link

codesandbox-ci bot commented Feb 13, 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.

@@ -0,0 +1,3 @@
# SSR Bundling for NPM packages with environment depended code parts

Choosing the right code based on the `export` section of the `react-textarea-autosize` module based on the backend server deployment target ('node' | 'webworker') and the client browser. No browser only code should be bundled with server only code.
Copy link
Owner

Choose a reason for hiding this comment

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

To me, it's not our fault that when target=webworker, the bundler picks the browser version, instead of worker version. So, it's our temporarily workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you suggest?

Copy link
Owner

Choose a reason for hiding this comment

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

Nothing. Sorry for the confusion. It's just a note so that we are on the same page.

@dai-shi
Copy link
Owner

dai-shi commented Feb 13, 2024

@aheissenberger do you know ci is failing?

@himself65 can you do a quick review?

"react-dom": "18.3.0-canary-4b2a1115a-20240202",
"react-server-dom-webpack": "18.3.0-canary-4b2a1115a-20240202",
"react-textarea-autosize": "^8.5.3",
"waku": "0.19.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"waku": "0.19.2"
"waku": "workspace:*"

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I think I should change other fixtures also, because they use monorepo actually

Copy link
Owner

@dai-shi dai-shi Feb 14, 2024

Choose a reason for hiding this comment

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

It's actually fine. If the version is the same, it will use the workspace version. (And, there's no way to opt-out this behavior, which is actually troublesome.)

Yeah, but to be safe, we can use workspace:* in all fixtures.

@@ -0,0 +1,28 @@
import { lazy } from 'react';
import { defineEntries } from 'waku/server';
import { Slot } from 'waku/client';
Copy link
Contributor

Choose a reason for hiding this comment

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

error here because you import a package from monorepo.

Solution is add it to references on tsconfig.json

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 your contribution!
As you may notice, we should cover more cases in e2e.

@dai-shi dai-shi merged commit b702b30 into dai-shi:main Feb 14, 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.

3 participants