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

update proxy-compare and re-export replaceNewProxy #48

Merged
merged 3 commits into from
Aug 14, 2022

Conversation

dai-shi
Copy link
Owner

@dai-shi dai-shi commented Jun 21, 2022

taking the update in dai-shi/proxy-compare#41

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 21, 2022

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.

Latest deployment of this branch, based on commit 323971e:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
React Configuration
React Typescript Configuration

Copy link

@janjangao janjangao left a comment

Choose a reason for hiding this comment

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

Look good to me, thanks for this great change

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 21, 2022

@hayond Please use the csb build seriously. (I mean, preferably in production, if possible.)
I'm not planning to merge this anytime soon, until I get really good sign/feedback/confirmation.

@janjangao
Copy link

Please use the csb build seriously. (I mean, preferably in production, if possible.)

ok sure, no problem, I also will write some examples in the CodeSandbox, though I don't use it very often, but I will try a more complex scenario to verify the changes.

@janjangao
Copy link

hi, @dai-shi I am trying use this csb in my daily work, but I got a type issue of the parameter of replaceNewProxy, how can I make it correct? usually, for using with other libs, I often import some types from them, extends, pass generic type or force case ;), but for this method, there isn't any type I can use from proxy-memoize, I am not a good type developer :), so maybe I missed some type usage.

image

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 22, 2022

Is TinyProxyPolyfill your function?
You can type it like original Proxy, and it should just pass.
(I need more info to help.)

https://codesandbox.io/s/react-typescript-forked-qn2ssx?file=/src/App.tsx
It works like this, but partly because it's any.

@janjangao
Copy link

Ah, I see, I try your writing, it works after I make the function as any... thanks.

TinyProxyPolyfill is my implementation of Proxy, since our work team has a limit hard requirement for dependencies, I don't want to import proxy-polyfill to make misunderstanding, though it can import as function, but need effort to explaining ^_^

@janjangao
Copy link

@dai-shi hihi, sorry for late response ^_^, I am stuck in my other work recently, today I continue to finish the integration of this lib, I found a error when I use codesandbox, it throws (0 , _proxyMemoize.default) is not a function, I also switch to use your csb, still has the same error, colud you help take a look? thanks.

can check this demo: https://codesandbox.io/s/react-typescript-forked-8j44uu?file=/src/App.tsx

@dai-shi
Copy link
Owner Author

dai-shi commented Jul 26, 2022

@janjangao
Copy link

Can you check this? https://github.com/dai-shi/proxy-memoize#importing-package

ah, let me try, seems I didn't read doc carefully.

@janjangao
Copy link

@dai-shi it works with this format import proxyMemoize from 'proxy-memoize/dist/index.modern.js';, but I have to ignore the type issue, is the error just happened on codesandbox? and for normal development I still can use esm import format, right?

@dai-shi
Copy link
Owner Author

dai-shi commented Jul 26, 2022

Ah, types doesn't work with hacked import path. You need to configure tsconfig.

        "baseUrl": ".",
        "paths": {
            "proxy-memoize/dist/index.modern.js": [
                "node_modules/proxy-memoize/dist/src/index.d.ts"
            ]
        }

https://codesandbox.io/s/goofy-goldwasser-yfo1zl?file=/tsconfig.json

If it happens with normal dev environment, we may need to look for a better solution.

@janjangao
Copy link

sure, for my daily usage, seems can work correctly, only found error in codesandbox, let me try more, thanks

@janjangao
Copy link

sure, for my daily usage, seems can work correctly, only found error in codesandbox, let me try more, thanks

oh, wrong conclusion... seems my daily usage also encountered this error, I will use the format you mentioned above, and add the configuration in the tsconfig.json as temporary solution. I will check later, hopefully we can find better solution.

image

@janjangao
Copy link

@dai-shi can work with this usage

import proxyMemoizeModule from 'proxy-memoize';
const proxyMemoize = proxyMemoizeModule.default;

I think it's caused by the index.modern.js, export default using export{s as default} instead of export default s

@dai-shi
Copy link
Owner Author

dai-shi commented Aug 3, 2022

I think it's not a problem: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export

The problem is a bundler (or transpiler) which doesn't support native ESM.
I think it doesn't use index.modern.js but index.umd.js instead.

@janjangao
Copy link

The problem is a bundler (or transpiler) which doesn't support native ESM.

yeah, maybe my working webpack version doesn't support this format, let me try the index.umd.js, thanks.

@dai-shi
Copy link
Owner Author

dai-shi commented Aug 3, 2022

I think webpack uses index.umd.js, even though the syntax is ESM import. So, there's a mismatch.

const proxyMemoize = require('proxy-memoize');

would work with CJS and index.umd.js. But, 🤷‍♂️ .

@dai-shi dai-shi marked this pull request as ready for review August 13, 2022 12:49
@dai-shi dai-shi changed the title update proxy-compare update proxy-compare and re-export replaceNewProxy Aug 13, 2022
@janjangao
Copy link

@dai-shi hi, I saw your updates, would you like to merge this MR to master and publish a new version as well?

since I used the CSB version in my daily work for some time, me and my colleagues used for some scenarios, seems no problem, and I prepare to put my tool (based on proxy-compare) to live usage, so I'd like to use regular version instead of CSB.

@dai-shi
Copy link
Owner Author

dai-shi commented Aug 14, 2022

seems no problem

This is important to hear.
If it works good for you and you find no problems, let's merge.

@janjangao
Copy link

If it works good for you and you find no problems, let's merge.

yeah, great, thanks!

@dai-shi dai-shi merged commit a8396e5 into main Aug 14, 2022
@dai-shi dai-shi deleted the fix/update-proxy-compare branch August 14, 2022 01:12
@dai-shi
Copy link
Owner Author

dai-shi commented Aug 14, 2022

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