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: Avoid require conflict with Cypress [#1405] #1406

Merged
merged 4 commits into from Mar 6, 2019

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Mar 5, 2019

Use derequire the rename all uses of require to _dereq_ to avoid the problem of Cypress overloading the require method.

Closes #1405

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: Stephen

@WilcoFiers WilcoFiers requested a review from a team as a code owner March 5, 2019 11:44
@stephenmathieson
Copy link
Member

This seems scary. I'd like to hear more about why this is necessary before OK'ing the code.

@WilcoFiers
Copy link
Contributor Author

@stephenmathieson Cypress seems to be replacing any variable with the name require with its own require method. I didn't dig into exactly why that is. I searched, but couldn't find any documentation about this. Renaming all uses of require to _dereq_ gets around that problem.

What is your concern with doing this? I realise that if derequire messes up, this could go wrong, but the same is true for browserify. That's why I used derequire, it's made to do this.

@WilcoFiers WilcoFiers changed the title fix: Avoid require conflict wity Cypress [#1405] fix: Avoid require conflict with Cypress [#1405] Mar 5, 2019
@stephenmathieson
Copy link
Member

Cypress seems to be replacing any variable with the name require with its own require method. I didn't dig into exactly why that is. I searched, but couldn't find any documentation about this.

Wow, OK. That's very strange. I wonder why that is 🤔

What is your concern with doing this?

I'm just worried about adding another layer of transformation to the codebase, especially since in this PR we're transforming code that we don't own/maintain (dot, axios, and es-promise). It's generally recommended that you do not run Babel/etc over your node_modules, so I'm of the same mindset with the "derequire" transformation.

However, since this is urgent, and the tests are all green, let's give this a go 😄

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

Not a blocker, just a thought -- would it make more sense to use this as a Browserify transform?

@stephenmathieson
Copy link
Member

stephenmathieson commented Mar 5, 2019

However, since this is urgent, and the tests are all green, let's give this a go 😄

I was wrong. Tests are now failing. Seems like some tests intermittently fail.

@WilcoFiers WilcoFiers merged commit 30aa570 into develop Mar 6, 2019
@WilcoFiers WilcoFiers deleted the cypress-derequire branch March 6, 2019 10:32
@jeeyyy jeeyyy mentioned this pull request Mar 6, 2019
1 task
@stephenmathieson
Copy link
Member

Based on #1427, it seems like using axe-core in Cypress still isn't working as expected.

@artemkochnev
Copy link

@stephenmathieson I just want to add that the issue I'm seeing only exists when using webpack to preprocess files. If I let Cypress manage things itself, using axe-core works without issue.

@YOU54F
Copy link

YOU54F commented Apr 5, 2019

This is an issue with webpack, I have switched to

https://github.com/cypress-io/cypress-browserify-preprocessor

which has resolved the issue.

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.

None yet

4 participants