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(webpack-preprocessor): Allow to pass custom webpack output #14599

Conversation

vtereshyn
Copy link

@vtereshyn vtereshyn commented Jan 16, 2021

User facing changelog

The user who uses @cypress/webpack-preprocessor has the ability to set his/her own Webpack publicPath

Additional details

Please, see #14592 for additional explanation.

How has the user experience changed?

Before:
image

After:
image

PR Tasks

  • [+] Have tests been added/updated?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 16, 2021

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Jan 16, 2021

CLA assistant check
All committers have signed the CLA.

@vtereshyn
Copy link
Author

Not sure who I have to tag in here, hope that @bahmutov will be the right person to address this.

@jennifer-shehane jennifer-shehane requested a review from a team January 19, 2021 05:14
Copy link
Contributor

@dmtrKovalenko dmtrKovalenko left a comment

Choose a reason for hiding this comment

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

I am not sure it will work with lazy-loaded modules (cypress has some limitations here – only allows to host 1 javascript file per spec). There is a test https://github.com/cypress-io/cypress/blob/develop/npm/react/cypress/component/advanced/lazy-loaded-suspense/lazy-loaded-suspense.spec.tsx that can check the desired behavior.

Can you please uncomment it and make sure that CI passed? If it passed – that would be awesome :)
Thank you

@vtereshyn
Copy link
Author

@dmtrKovalenko I am pretty sure it will work with Lazy loaded components as I showed in my screenshots.

Here is my usage: I have a component that wrapped into HOC which lazy loads another component (popup) and I got a Webpack error, not React or Cypress.

Right now as a fix I'm editing node_modules/@cypress/ folder to proceed with my work.

Also, sorry, I didn't get what particularly should I uncomment? I see that the code you mentioned is uncommented.

@dmtrKovalenko
Copy link
Contributor

Cool if so, sorry just remove the .skip on test and the coment

@vtereshyn
Copy link
Author

@dmtrKovalenko ok, will do. But my fix is not about that at all. My fix is particularly about webpack-preprocessor and webpack error, not about Cypress and React.Lazy. I didn't change any code in Cypress core

@dmtrKovalenko
Copy link
Contributor

dmtrKovalenko commented Jan 19, 2021

I understand this, but PR title says that React.lazy breaks the webpack-preprocessor code, right? @cypress/react is using webpack-preprocessor now so we can check that PR works just fine?

Or did I miss something?

@@ -151,6 +151,20 @@ describe('webpack preprocessor', function () {
})
})

it('adds output options when user output options are non-default', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't really check that when we have dynamic import somewhere that webpack doesn't crash. It only checks that specific options passed to webpack config.
What do you think about adding more complex test that shows the desired behavior if @cypress/react tests won't work?

@vtereshyn
Copy link
Author

vtereshyn commented Jan 19, 2021

Probably this is incorrect to name PR as I named it here. I named it React lazy loads breaks webpack config just because I faced this error during Lazy loading in one of my components. This PR should be named like Allow Cypress user to pass own Webpack output configuration. Basically, this fix is about dynamic loading as well, just because Webpack doesn't understand the path inside the code.
Ex:
const Component = import('../../../${YourComponentName}'

Please note, this is just "pseudo code"

@dmtrKovalenko
Copy link
Contributor

Cool, thx its more clear now. Then it should be not fix: but feat: as well

@vtereshyn
Copy link
Author

Cool, thx its more clear now. Then it should be not fix: but feat: as well

Agreed, do you want me to create new PR with using new branch?

@dmtrKovalenko
Copy link
Contributor

I think just rename the title

@dmtrKovalenko dmtrKovalenko changed the title fix: webpack-preprocessor-lazy-load - React lazy load breaks webpack config feat(webpack-preprocessor): Allow to pass custom webpack output Jan 19, 2021
@vtereshyn
Copy link
Author

@dmtrKovalenko sounds good, thanks

@vtereshyn
Copy link
Author

@dmtrKovalenko ok, so do you want me to do something else? Write Cypress tests?

@dmtrKovalenko
Copy link
Contributor

It may be nice to add e2e tests in webpack-preprocessor that shows how it works, but maybe that is not needed. :)
Probably let's wait for CI to pass

@vtereshyn
Copy link
Author

vtereshyn commented Jan 19, 2021

Ok, thanks! I see some weird behavior with some checks, that related to code I haven't touched. Not sure why it happened.

@vtereshyn
Copy link
Author

Hey @dmtrKovalenko
I'm sorry, but I can't get what's going on CI because I have executed test script locally and everything was good. Not sure what should I do next and how can I reproduce CI issue but locally. Thanks!

@dmtrKovalenko
Copy link
Contributor

@vtereshyn this error is coming from npm/react package mostly.
Just do the cd npm/react && yarn cy:open

@vtereshyn vtereshyn force-pushed the fix/webpack-preprocessor-lazy-load branch from f03fb78 to 685f11e Compare January 20, 2021 13:54
@vtereshyn
Copy link
Author

image

@vtereshyn
Copy link
Author

image

I am not sure whats wrong on CI then

@vtereshyn
Copy link
Author

@dmtrKovalenko any thoughts?

@vtereshyn
Copy link
Author

well, I am not getting any responses, so I'm gonna close this PR. @dmtrKovalenko @jennifer-shehane

@dmtrKovalenko
Copy link
Contributor

Do not close it, I'll take a look once get some free time

@lmiller1990
Copy link
Contributor

@vtereshyn when you run the tests locally you may need to build npm/webpack-preprocessor manually - either run yarn watch in npm/webpack-preprocessor or just with yarn build. That might explain why things work locally (you ran the tests without your change) but CI builds everything prior to running the tests.

@vtereshyn
Copy link
Author

@lmiller1990 thanks for your suggestion. I did yarn build before running tests. And looks like I got all test passed locally again.

…config

fix: webpack-preprocessor-lazy-load - React lazy load breaks webpack config
@vtereshyn vtereshyn force-pushed the fix/webpack-preprocessor-lazy-load branch from 685f11e to f09cd75 Compare January 22, 2021 08:15
@lmiller1990
Copy link
Contributor

Will take a look!

@lmiller1990
Copy link
Contributor

I ran the tests locally and reproduced the error once. But then I ran them again and it all seemed fine. I'll try re-running CI and see what happens.

@vtereshyn
Copy link
Author

@lmiller1990 thank you

@vtereshyn vtereshyn closed this Feb 12, 2021
@vtereshyn vtereshyn deleted the fix/webpack-preprocessor-lazy-load branch February 12, 2021 12:54
@am-a am-a mentioned this pull request Jun 23, 2021
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.

Mount Component that uses React.lazy throws webpack error
4 participants