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

Externalize @mswjs/* packages in certain builds #913

Merged

Conversation

patrickmcdougle-okta
Copy link
Contributor

@patrickmcdougle-okta patrickmcdougle-okta commented Sep 13, 2021

PR mentioned in #660 (comment)

Maybe fixes: #905

More work to be added possibly: #851 (comment) (which would likely fix #912) Not doing this here.

@patrickmcdougle-okta

This comment has been minimized.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 13, 2021

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 401e68c:

Sandbox Source
MSW React Configuration

@patrickmcdougle-okta patrickmcdougle-okta marked this pull request as ready for review September 13, 2021 20:46
@patrickmcdougle-okta
Copy link
Contributor Author

patrickmcdougle-okta commented Sep 13, 2021

Let me know if you need me to clean up the history (squash) and / or let me know if you want me to handle the debug dependency problem in this body of work as well.

Thanks!

@patrickmcdougle-okta patrickmcdougle-okta changed the title Externalize interceptors in esm build Externalize @mswjs/interceptors in esm build Sep 13, 2021
rollup.config.ts Outdated Show resolved Hide resolved
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

Hey, @patrickmcdougle-okta. Thank you for preparing this, the changes look great!

I've had a few comments and would like to know your opinion. I think this is almost ready to go, just a few things left. Looking forward to releasing this!

Regarding the debug issue, let's address it in a separate pull request. I feel it may involve some changes in the biuld process of the interceptors library.

@patrickmcdougle-okta
Copy link
Contributor Author

Sorry for the delay, I was off yesterday. I'll make the proposed changes today. Thanks for your patience and your review!

@patrickmcdougle-okta
Copy link
Contributor Author

I rebased to pull in the current tip of master, but kept a separate new commit for easy reviewing. Let me know if you need me to squash anything.

@patrickmcdougle-okta patrickmcdougle-okta changed the title Externalize @mswjs/interceptors in esm build Externalize @mswjs/* packages in certain builds Sep 15, 2021
rollup.config.ts Outdated Show resolved Hide resolved
@kettanaito
Copy link
Member

kettanaito commented Sep 15, 2021

The current CI fails on the init integration test:

> msw@0.35.0 postinstall /root/release/tmp/auto-update-worker/node_modules/msw
> node -e "try{require('./config/scripts/postinstall')}catch(e){}"

/root/release/tmp/auto-update-worker/node_modules/inquirer/lib/prompts/rawlist.js:126
      this.selected = this.selected ?? -1;
                                     ^

SyntaxError: Unexpected token '?'
    at wrapSafe (internal/modules/cjs/loader.js:1054:16)
    at Module._compile (internal/modules/cjs/loader.js:1102:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1158:10)
    at Module.load (internal/modules/cjs/loader.js:986:32)
    at Function.Module._load (internal/modules/cjs/loader.js:879:14)
    at Module.require (internal/modules/cjs/loader.js:1026:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at Function.promptModule.restoreDefaultPrompts (/root/release/tmp/auto-update-worker/node_modules/inquirer/lib/inquirer.js:65:36)
    at Object.inquirer.createPromptModule (/root/release/tmp/auto-update-worker/node_modules/inquirer/lib/inquirer.js:72:16)
    at Object.<anonymous> (/root/release/tmp/auto-update-worker/node_modules/inquirer/lib/inquirer.js:84:28)

Seems related to #916.

Edit: You can ignore this failure, it won't happen anymore. The issue was fixed by the underlying dependency that introduced it in the first place.

Since we want to allow interceptors to have bug fixes without re-releasing msw,
we need to externalize this module such that it is resolved in the consumers
node_modules directory instead of bundled into this build.
@patrickmcdougle-okta
Copy link
Contributor Author

I rebased again to pull in the current tip of master, but kept another separate commit for easy reviewing. Let me know if you need me to squash anything.

Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

Thank you for this improvement, @patrickmcdougle-okta!

Collaborating with you on this was great. Let me know if you are interested in helping us with the project, there are always some tasks to work on.

Meanwhile, let’s have this one merged.

@kettanaito kettanaito merged commit 6decc60 into mswjs:master Sep 16, 2021
@patrickmcdougle-okta patrickmcdougle-okta deleted the externalize-interceptors branch September 16, 2021 16:18
@patrickmcdougle-okta
Copy link
Contributor Author

Sorry to bother on a closed PR...Will there be a release with this code included soon? It fixes a bug I'm seeing in a product I'm building and I'd like to avoid maintaining a fork.

@kettanaito
Copy link
Member

No worries. I'd like to include a few more bug fixes in the next release. There's no ETA, but I was planning on releasing the next version somewhere in October.

@misha-erm
Copy link

+1 for releasing this. I can't use msw right now because of it.

I described my use-case here #796 (comment)

Thanks in advance

@msutkowski
Copy link
Member

@patrickmcdougle-okta @MikeYermolayev I'd recommend using the CSB build until the next release if you're in a bind. If you weren't aware, you can install as shown here: https://ci.codesandbox.io/status/mswjs/msw/pr/913/builds/169956

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants