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: make vite-dev-server work on windows #16103

Merged
merged 5 commits into from
Apr 22, 2021
Merged

fix: make vite-dev-server work on windows #16103

merged 5 commits into from
Apr 22, 2021

Conversation

elevatebart
Copy link
Contributor

@elevatebart elevatebart commented Apr 20, 2021

User facing changelog

Before this PR loading the testing client did not work on Windows.
See issue.

Additional details

There was a bunch of backslashes mixed in the urls.

How has the user experience changed?

@cypress/vite-dev-server is now working on windows

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 20, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Apr 20, 2021



Test summary

9494 0 111 3Flakiness 0


Run details

Project cypress
Status Passed
Commit f19bcc1
Started Apr 21, 2021 5:14 PM
Ended Apr 21, 2021 5:27 PM
Duration 13:12 💡
OS Linux Debian - 10.8
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Few nit picks

@@ -16,14 +18,18 @@ export const makeCypressPlugin = (
): Plugin => {
let base = '/'

const posixSupportFilePath = supportFilePath ? resolve(projectRoot, supportFilePath).replace(OSSepRE, '/') : undefined

const normalizedSupportFilePath = posixSupportFilePath ? `${base}@fs/${posixSupportFilePath}` : undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity where does this @fs come from? I've seen it before for the node_modules but never really understood it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I could use a function from vite to format those.
Let me dig a little deeper.

Overall it's an abstraction on the vite server to call on the file system.
If a module path starts with @fs/, return the file from the filesystem.

The server was returning the files just fine on Mac, but windows could not access them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proper functions to condition file calls in Vite are not yet exposed.
See fileToUrl() in assets.ts.
Maybe we can get Vite to expose some kind of util library?

Until then its the only way since we have our HTML in the wrong place (within node_modules)

Copy link
Contributor

Choose a reason for hiding this comment

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

All good, just curious - good to know this comes from Vite.

return {
name: pluginName,
enforce: 'pre',
config (_, env) {
if (env) {
return {
define: {
'import.meta.env.__cypress_supportPath': JSON.stringify(supportFilePath ? resolve(projectRoot, supportFilePath) : undefined),
'import.meta.env.__cypress_supportPath': JSON.stringify(normalizedSupportFilePath),
Copy link
Contributor

Choose a reason for hiding this comment

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

What does JSON.stringify do here? Do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just like with webpack define plugin, the strings on the left are meant to be replaced in the code literally.

So if they contain any special character, I want it to be escaped automatically.
This way when they are eval-ed within the code, the special characters are kept.

I hope it is clear.

See https://webpack.js.org/plugins/define-plugin/

Copy link
Contributor

Choose a reason for hiding this comment

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

ok!

@@ -40,7 +46,7 @@ export const makeCypressPlugin = (
tag: 'script',
injectTo: 'body',
attrs: { type: 'module' },
children: `import(${JSON.stringify(INIT_FILEPATH)})`,
children: `import(${JSON.stringify(`${base}@fs/${INIT_FILEPATH}`)})`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need JSON.stringify. I was able to remove something similar in webpack-dev-server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure you should have removed it from webpack-dev-server either.
Do test hard cases with special char in the folder name:

  • Oma's sister.
  • The "Best" project
  • The back\slash

In those cases we will end up with either an unterminated string or some characters that are removed from the string.
It is not my intention.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooo ok. Maybe we should revert that change I made. :|

@lmiller1990 lmiller1990 self-requested a review April 22, 2021 01:43
@elevatebart elevatebart merged commit a380d02 into master Apr 22, 2021
tgriesser added a commit that referenced this pull request Apr 26, 2021
* develop: (49 commits)
  fix: `cy.type()` should not change the value attr of button-like inputs. (#16154)
  fix: properly detect `cy.intercept(url, routeMatcher, handler)` overload (#16167)
  fix: consider multiple routes when looking for aliases (#16180)
  fix: pass contextIsolation: true (#16165)
  chore: fix failing "should handle aborted requests" test (#16170)
  feat(issue-3741): added keyboard support for folder (#15648)
  fix(webpack-dev-server): remove hard dependency on html-webpack-plugin v4  (#16108)
  chore(deps): downgrade electron to v12.0.0-beta.14 (#16113)
  fix: accept absolute paths in vite dev server (#16148)
  fix: cy.then shows wrong type when collection of HTMLElement's is provided (#15869)
  fix: do not treat utf8 requests as binary (#15946)
  chore: fix types
  docs: fix broken links for @cypress/react example recipes (#15674)
  update circle yml
  ignore undefined beforeEach
  fix: make vite-dev-server work on windows (#16103)
  chore: add triple slash reference
  chore: remove conflicting types
  chore: rebuild yarn lock
  resolve conflicts in master(fe0b63c) and develop
  ...
@emilyrohrbough emilyrohrbough deleted the fix/vite-widows branch August 1, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants