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

[WINDOWS]: rollup-plugin-node-builtins path only supports POSIX functions #22029

Closed
sync-by-unito bot opened this issue Jun 2, 2022 · 4 comments
Closed
Assignees

Comments

@sync-by-unito
Copy link

sync-by-unito bot commented Jun 2, 2022

Current Behavior

The current package that we are using to inject the path package into the browser, is not 1:1 with nodejs path, having some differences like only supporting POSIX functions.

This can become an issue for the paths that we are building on the driver.

related with https://github.com/cypress-io/cypress/pull/21278

Desired Behavior

We should be able to have a solution that when we use the path, we are safe covering the correct OS

Reproducible Steps

  1. Open a spec on IDE from the app
  2. Check the absolute path sent on the variables of the mutation

┆Issue is synchronized with this Jira Bug by Unito
┆author: Alejandro Estrada
┆friendlyId: UNIFY-1700
┆priority: High
┆sprint: Fast Follows 1
┆taskType: Bug

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Jun 2, 2022

➤ Mark Noonan commented:

Alejandro Estrada does removing the extra / in https://github.com/cypress-io/cypress/pull/21278/files ( https://github.com/cypress-io/cypress/pull/21278/files|smart-link ) provide a tempfix for the issue? Are there other issues related to the same thing, and do we need to change the package before launch?

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Jun 2, 2022

➤ Alejandro Estrada commented:

Removing the extra / fixes the issue, I have not seen related issues, but worth to keep track of that issue that maybe can add some overhead in the future - we should be good for the launch as it is

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Jun 2, 2022

➤ Zach Williams commented:

I’ve looked around and can’t find any packages that might help us with this. The popular path browser packages (1 ( browserify/path-browserify#1 ), 2 ( https://github.com/calvinmetcalf/rollup-plugin-node-builtins )) don’t support windows, so to get full support we would have to roll our own. We don’t have many usages of path in the browser, mostly in driver and the API mostly used is baseName. I’m ok with the behavior we have now for 10.0

@BlueWinds
Copy link
Contributor

Discussed this with @estrada9166, and together came to the conclusion that everything needed here has already been done in https://github.com/cypress-io/cypress/pull/21278/files. That fixed the primary issue, and I've reviewed the code as best I can to verify that there aren't any other instances we've missed where this will be a problem.

Closing the ticket without a PR, nothing more to see here.

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

No branches or pull requests

2 participants