fix(rspeedy/core): deno compat #1412
Conversation
🦋 Changeset detectedLatest commit: c59e9e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
📝 WalkthroughWalkthroughA new changeset was added to document a patch update for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
colinaaa
left a comment
colinaaa
left a comment
There was a problem hiding this comment.
Thank you for your PR! Just a few comments
| * @returns - The `unregister` function. | ||
| */ | ||
| export function register(options) { | ||
| // Check for Deno environment first |
There was a problem hiding this comment.
We already have a hasNativeTSSupport function at:
which is based on process.features.typescript.
Although deno does not support process.features.typescript yet :)
But it would be great to have the similar logic together.
There was a problem hiding this comment.
I tried just adding the Deno logic to hasNativeTSSupport but it resulted in the same error when trying to run with Deno because the current logic assumes nativeTsSupport runtimes still support module.register.
I ended up using hasNativeTSSupport in a conditional for running register.
This only meant I had to change the commonConfig.js imports to commonConfig.ts in web-tests because these specific tests are run directly from the TypeScript source and not compiled into JS.
All other tests and libraries built perfectly fine with no changes and all tests pass.
I understand this change may be too much for just adding Deno support and I really did want to keep it isolated but that would have meant using the original change or making the conditional specific to only Deno. Please let me know if this meets your standards.
There was a problem hiding this comment.
We are using register without condition to fix #1406.
I do think it would be necessary to keep this way since there could be a lot of user that are doing this way (import with .js for a .ts file) since this is the default of TypeScript.
There was a problem hiding this comment.
I would prefer something like this:
const unregister = shouldUseNativeImport(configPath)
? /** noop */ () => void 0
: register({
load: !hasNativeTSSupport(),
resolve: true,
})
function shouldUseNativeImport(configPath: string): boolean {
return isJavaScriptPath(configPath) || isDeno()
}
function isDeno(): boolean {
// @ts-expect-error - Deno global may not be defined in Node.js
if (typeof Deno !== 'undefined' || process.versions?.deno) {
return true
}
return false
}wdyt
There was a problem hiding this comment.
That's a perfect solution! Sorry I tried a bunch of combinations of isJavaScriptPath, hasNativeTSSupport and the Deno logic but this one is clearly the most logical. Just updated it to reflect this and removed the web-test changes. Let me know what you think.
colinaaa
left a comment
colinaaa
left a comment
There was a problem hiding this comment.
We need to be compatible with using .js to import .ts file. Please consider make a change to shouldUseNativeImport instead.
| * @returns - The `unregister` function. | ||
| */ | ||
| export function register(options) { | ||
| // Check for Deno environment first |
There was a problem hiding this comment.
We are using register without condition to fix #1406.
I do think it would be necessary to keep this way since there could be a lot of user that are doing this way (import with .js for a .ts file) since this is the default of TypeScript.
| * @returns - The `unregister` function. | ||
| */ | ||
| export function register(options) { | ||
| // Check for Deno environment first |
There was a problem hiding this comment.
I would prefer something like this:
const unregister = shouldUseNativeImport(configPath)
? /** noop */ () => void 0
: register({
load: !hasNativeTSSupport(),
resolve: true,
})
function shouldUseNativeImport(configPath: string): boolean {
return isJavaScriptPath(configPath) || isDeno()
}
function isDeno(): boolean {
// @ts-expect-error - Deno global may not be defined in Node.js
if (typeof Deno !== 'undefined' || process.versions?.deno) {
return true
}
return false
}wdyt
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #1412 will not alter performanceComparing Summary
|
React Example#3678 Bundle Size — 235.14KiB (0%).c59e9e7(current) vs 5450d00 main#3660(baseline) Bundle metrics
|
| Current #3678 |
Baseline #3660 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
156 |
156 |
| Duplicate Modules |
63 |
63 |
| Duplicate Code |
45.94% |
45.94% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #3678 |
Baseline #3660 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
89.38KiB |
89.38KiB |
Bundle analysis report Branch knotbin:main Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#3668 Bundle Size — 342.17KiB (0%).c59e9e7(current) vs 5450d00 main#3652(baseline) Bundle metrics
|
| Current #3668 |
Baseline #3652 |
|
|---|---|---|
| Initial JS |
142.14KiB |
142.14KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
7 |
7 |
| Assets |
7 |
7 |
 Modules |
211(-0.47%) |
212 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
4.02% |
4.02% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #3668 |
Baseline #3652 |
|
|---|---|---|
| JS |
227.22KiB |
227.22KiB |
| Other |
83.11KiB |
83.11KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch knotbin:main Project dashboard
Generated by RelativeCI Documentation Report issue
|
Hi @knotbin, I still find it not working with Deno even with this change is merged. I've run: which results in: I'm not fimiliar with Deno, but as you can see in |
|
Hi @colinaaa, I'm sorry this didn't fully fix the issue. This issue is why the PR initially forced rspeedy into unmanaged mode. Rspeedy now works in Deno if you run More details below but the reason I didn't catch this when I made the PR was that I wasn't getting this error when I ran in the Deno installs dependencies into the node-modules folder in a different pattern than node would but symlinks them to where node would expect them to be. I assume this symlinking has been specifically done to work with node and that's why running It seems like the problem is specifically with resolving dependencies from that specific endpoint ( I'll do some digging to make this work without |
|
Thanks for your explanation @knotbin! For canary versions, we use npm aliases: {
"dependencies": {
"@lynx-js/chunk-loading-webpack-plugin": "npm:@lynx-js/chunk-loading-webpack-plugin-canary@version"
}
}I'm not sure if the error is caused by npm alias, since it changes the package name. It seems like we can only find |
|
@colinaaa I've traced the issue to the fact that start.ts (called by rspeedy.js) seems to override the runtime's module resolution and implement a custom resolver using very jank filesystem logic to load the Trying to reimplement an import seems like a surefire way to introduce inconsistencies in resolution and bugs like the Deno one here. Package managers like PNPM, Deno, Bun, Yarn and even NPM now already handle resolving workspace dependencies gracefully. I might be missing something but the I'm probably missing something, because the start.ts file is just part of the initial commit so it's hard for me to see why the functionality was added, so me know if there's something I'm not thinking of. |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/[email protected] ### Patch Changes - Supports `recyclable` attribute in `<list-item>` to control whether the list item is recyclable. The `recyclable` attribute depends on Lynx Engine 3.4 or later. ([#1388](#1388)) ```jsx <list-item recyclable={false} /> ``` - feat: Support using a host element as direct child of Suspense ([#1455](#1455)) - Add profile in production build: ([#1336](#1336)) 1. `diff:__COMPONENT_NAME__`: how long ReactLynx diff took. 2. `render:__COMPONENT_NAME__`: how long your render function took. 3. `setState`: an instant trace event, indicate when your setState was called. NOTE: `__COMPONENT_NAME__` may be unreadable when minified, setting `displayName` may help. - Add `onBackgroundSnapshotInstanceUpdateId` event on dev for Preact Devtools to keep the correct snapshotInstanceId info. ([#1173](#1173)) - fix: Prevent error when spreading component props onto an element ([#1459](#1459)) - fix: Correctly check for the existence of background functions in MTS ([#1416](#1416)) ```ts function handleTap() { "main thread"; // The following check always returned false before this fix if (myHandleTap) { runOnBackground(myHandleTap)(); } } ``` ## @lynx-js/[email protected] ### Patch Changes - Remove the experimental `provider` option. ([#1432](#1432)) - Add `output.filename.wasm` and `output.filename.assets` options. ([#1449](#1449)) - fix deno compatibility ([#1412](#1412)) - Should call the `api.onCloseBuild` hook after the build finished. ([#1446](#1446)) - Bump Rsbuild v1.4.15. ([#1423](#1423)) - Support using function in `output.filename.*`. ([#1449](#1449)) ## [email protected] ### Patch Changes - Support ESLint for ReactLynx templates ([#1274](#1274)) ## @lynx-js/[email protected] ### Patch Changes - Updated dependencies \[[`c8ce6aa`](c8ce6aa)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Fix the `Package subpath './compat' is not defined by "exports"` error. ([#1460](#1460)) ## @lynx-js/[email protected] ### Patch Changes - Fix `GlobalEventEmitter` type definition, the `emit(eventName: string, data: unknown)` function should recevie an array typed `data` and pass as param list of listeners. ([#1479](#1479)) ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - Updated dependencies \[]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - fix: when `onNativeModulesCall` is delayed in mounting, the NativeModules execution result may be undefined. ([#1457](#1457)) - fix: `onNativeModulesCall` && `onNapiModulesCall` use getter to get. ([#1466](#1466)) - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. ## @lynx-js/[email protected] ### Patch Changes - feat: add autocomplete attribute support for x-input component ([#1444](#1444)) Implements autocomplete attribute forwarding from the x-input custom element to the internal HTML input element in the shadow DOM. This enables standard browser autocomplete functionality for x-input elements. - Add referrerpolicy attribute support to x-image web component ([#1420](#1420)) - Updated dependencies \[]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## [email protected] ## @lynx-js/[email protected] ## @lynx-js/[email protected] ## @lynx-js/[email protected] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Thank you @knotbin for your reply! Sorry for the delay :)
Both the idea and the implementation came from It seems like the implementation is not good enough since it simulate the Node.js resolve. |
3 participants
Summary by CodeRabbit (edited by me)
closes #1393
Checklist
Summary by CodeRabbit
Bug Fixes
Chores