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

Avoid relying on Node's fs when running in browser-based JSEnv #247

Closed
keynmol opened this issue Nov 7, 2020 · 6 comments · Fixed by #376
Closed

Avoid relying on Node's fs when running in browser-based JSEnv #247

keynmol opened this issue Nov 7, 2020 · 6 comments · Fixed by #376

Comments

@keynmol
Copy link

keynmol commented Nov 7, 2020

Problem

MUnit relies on Node.js' fs module to read the source location when an assertion fails.

This is only possible if SJS if told to emit CommonJSModule or ESModule:

 scalaJSLinkerConfig ~= (_.withModuleKind(ModuleKind.CommonJSModule)),

Issue is that one cannot use such modules in browser-based JSEnv, filesystem doesn't exist there. And tests fail with an exception:

[error] munit.internal.JSFs$ needs to be imported from module 'fs' but module support is disabled
[error]   called from private java.nio.file.Files$.$anonfun$readAllBytes$1(java.nio.file.Path)[byte
[error]   called from java.nio.file.Files$.readAllBytes(java.nio.file.Path)[byte
[error]   called from java.nio.file.Files$.readAllLines(java.nio.file.Path)java.util.List
[error]   called from private munit.internal.console.Lines.$anonfun$formatLine$1(java.nio.file.Path)[java.lang.String
[error]   called from munit.internal.console.Lines.formatLine(munit.Location,java.lang.String,munit.Clues)java.lang.String
[error]   called from munit.internal.console.Lines.formatLine(munit.Location,java.lang.String)java.lang.String

Proposed solution

@sjrd and @olafurpg propose a fallback solution, akin to

try { js.Dynamic.global.require("fs") } catch { ... }

to avoid having fs a hard dependency for MUnit.

Reference material

@raquo
Copy link

raquo commented Jun 6, 2021

@olafurpg Are you planning to solve this? If I understanding correctly, there is no workaround for testing projects that need JS DOM, right? Seems that the usefulness of Scala.js support is pretty limited if that's the case...

I was hoping to switch from ScalaTest to MUnit, but I have a lot of frontend tests that need (Test / requireJsDomEnv) := true.

@olafurpg
Copy link
Member

olafurpg commented Jun 7, 2021

No objections from me if somebody wants to implement the proposed fix. The fs module is optional, it’s only used to enrich error messages. We can merge the fix even with no tests to unblock this usecase.

@olafurpg
Copy link
Member

olafurpg commented Jun 7, 2021

What are the steps to reproduce this error? I tried to reproduce in the MUnit repo with the diff here but the tests run without errors https://github.com/scalameta/munit/compare/main...olafurpg:js-node-fs?expand=1

@olafurpg
Copy link
Member

olafurpg commented Jun 7, 2021

nvm, I was able to reproduce by removing the linker config settings.

@olafurpg
Copy link
Member

olafurpg commented Jun 7, 2021

I opened #376 replacing the usage of JS imports with js.Dynamic.global.require("fs"). @raquo @keynmol could you please try out the PR to validate that it fixes this issue?

@armanbilge
Copy link
Contributor

I'm working on cross-building http4s/http4s#4938 to Scala.js and ran into this issue too when trying to run the test suite on Firefox instead of Node.js. Very very glad to see a fix in the works.

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 a pull request may close this issue.

4 participants