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 specific fixes for npm + js builds and testing #4899

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ambiguousname
Copy link

@ambiguousname ambiguousname commented May 13, 2024

One commit is from ambiguousname/diplomat@587865a (it also has an associated pull request in rust-diplomat/diplomat#474)

In this repo specifically, I found an issue with webpack.config.js:
It's probably a \ vs / problem.

new URL('dist', import.meta.url).pathname doesn't work for Windows. After futzing around with different approaches, I found the simplest was just removing file:/// from the full href.

I'd be curious to know if this works on other systems, and I'm open to workshopping a cross-platform solution if not!

It's probably a \ vs / problem.

new URL('dist', import.meta.url).pathname doesn't work for Windows. After futzing around with different approaches, I found the simplest was just removing file:/// from the full href.

I'd be curious to know if this works on other systems, and I'm open to workshopping a cross-platform solution if not!
@ambiguousname ambiguousname requested review from Manishearth and a team as code owners May 13, 2024 22:13
Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

Happy with the webpack change, let's discuss the node change on the diplomat issue.

Copy link
Member

Choose a reason for hiding this comment

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

this is generated code so you cannot make changes to it without failing CI (which checks for consistency).

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

2 participants