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: sync fs esm library after patching cjs version #591

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

jbedard
Copy link
Member

@jbedard jbedard commented Nov 5, 2022

For example as a test... without the syncBuiltinESMExports the last log (5) is false:

    import('fs').then(importFS => {
        console.log(1, origReaddirSync === fs.readdirSync)          // pre patched
        console.log(2, origReaddirSync === importFS.readdirSync)    // pre patch
        patchfs(fs, roots)
        console.log(3, origReaddirSync !== fs.readdirSync)          // post patched
        console.log(4, origReaddirSync === importFS.readdirSync)    // pre synced
        module.syncBuiltinESMExports()
        console.log(5, origReaddirSync !== importFS.readdirSync)    // post sync
    });

@matthewjh
Copy link

@jbedard, will this address #362?

@jbedard
Copy link
Member Author

jbedard commented Nov 9, 2022

No it does not.

In #362 the fs methods are destructed using cjs: {...} = require('fs'). This PR addresses the case where the fs methods are destructed using esm: import {...} from 'fs', and we haven't actually observed this esm case anywhere so I'm not sure if this will actually land.

@matthewjh
Copy link

I see. Well, it seems useful to me, just in case someone in 1p code or a library decides to do this.

What are the downsides to merging?

@jbedard
Copy link
Member Author

jbedard commented Nov 12, 2022

Just the possibility of breaking something. I'd prefer merging and giving it a shot though...

@jbedard jbedard marked this pull request as ready for review November 12, 2022 21:57
Copy link
Member

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

🌮

@gregmagolan gregmagolan merged commit 58e3d60 into aspect-build:main Nov 16, 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.

3 participants