js_run_devserver symlinks scoped node_modules to bazel-out instead of runfiles (unscoped node_modules are already linked to bazel-out)

[gregjacobs]

Fix for #1204

It seems there was code in js_run_devserver.mjs that symlinked non-scoped node modules (such as 'lodash') to the bazel-out directory (instead of the runfiles directory), but it didn't handle scoped node modules like '@babel/core'

This fix solves the issue of two node_modules trees seen by bundlers when using scoped node modules

---

Type of change

• Bug fix (change which fixes an issue)

Test plan

• Automated testing: See this comment: js_run_devserver symlinks scoped node_modules to bazel-out instead of runfiles (unscoped node_modules are already linked to bazel-out) #1210 (comment)

[CLAassistant]

All committers have signed the CLA.

[jbedard]

Did you have an example we can put in a test or in the examples directory?

[gregjacobs]

Hmm, the only example I have is the reproduction I made here: https://github.com/gregjacobs/rules-js-devserver-multiple-react-reproduction. Check out the 'packages' directory which has a library called '@my-org/button', and a webpack 'app'.

The '@my-org/button' component only shows up when running the app if this multiple node_modules trees issue is solved (the button says "If you see me, you have fixed the issue!"). See here: https://github.com/gregjacobs/rules-js-devserver-multiple-react-reproduction/blob/main/packages/app/src/app.tsx

Was thinking about a test which launches the webpack app and then looks for the button to be on the screen using puppeteer or playwright. That would be a good full integration test. But otherwise, could possibly have a lower-level test which just checks that both unscoped and scoped node_modules symlink to the bin directory rather than the runfiles directory? Or do you have another recommendation?

[gregjacobs]

Ok so I came up with a test for this actually, although it may be a little complex and may not be immediately clear exactly what it's testing.

The test basically replicates the reproduction that I made. It creates a webpack app with a single component, and then uses Puppeteer to check that the component rendered on the page. If the component doesn't display, one reason could be that there are multiple copies of React in the bundle and the component is throwing an error by trying to use a hook. However, the webpack compilation could also be broken for other reasons as well (of which we'll see the error from the test script).

I checked that the test properly fails when removing the fix I had made in js_run_devserver.mjs, and then re-added the fix and the test passes.

Totally open to simpler ideas if you think of any though!

[gregjacobs]

Update: ended up writing both tests: explicit tests for the node_modules' symlinks, and an integration test to make sure a Webpack compilation works within js_run_devserver. Happy with this version now: js/private/test/js_run_devserver/js_run_devserver.spec.mjs

[gregjacobs]

Hey @gregmagolan, can you take a quick look at this PR? This is basically the extension of what you did in #1043, but just to handle node modules like '@org/pkg' in addition to 'pkg'

[gregjacobs]

Btw if you feel the webpack compilation in the test and all is too complex, I can remove. I think just having the test that checks the node_modules symlinks at this point is good enough

[jbedard]

That is a lot of code and a complicated test for such a simple fix, so I'm not sure if it's worth it. The tests in js/private/test are fairly simple and more like what I would hope for, I'm not sure if that's really possible with something more complex like a devserver though?

If we can't find an easier test I think we can probably live without one. WDYT? Either way thank you for giving the test a try! 👍

[gregjacobs]

No worries! I'll simplify to just leave the test of the symlinks then and remove the webpack test 👍 Will ping back in a bit

[gregmagolan]

Oh nice. Thanks for fixing this! I overlooked the @scoped/package case entirely.

For testing this change I think it would be sufficient to export the function export function isNodeModulePath(srcPath) and test just that function with a unit test. The fix is straight forward so I'm not too worried about adding the larger test right away if you wanted to simplify the PR further.

[gregjacobs]

For testing this change I think it would be sufficient to export the function export function isNodeModulePath(srcPath) and test just that function with a unit test. The fix is straight forward so I'm not too worried about adding the larger test right away if you wanted to simplify the PR further.

@gregmagolan Hah, just saw this after I pushed my latest simplification 😄 I did go with an integration test to ensure the proper symlinks get all the way to the file system inside the devserver's sandbox. Check it out and let me know what you think :)

Best,
Greg

[jbedard]

I think I'd say the same as @gregmagolan, I feel like modifying the rule to expose things is a bit much for what is really a fairly simple change.

If the util method is exported can we do something super simple similar to other tests in js/private/test? js/private/test/no_copy_to_bin might be a reasonable example with a single .js and js_test rule...?

[gregjacobs]

Hmm, the only problem with just exporting the util method and testing it directly is that the js_run_devserver.mjs script could be refactored in the future to potentially create the symlinks incorrectly, even though the isNodeModulePath() function is correct.

Testing that the correct symlinks make it to the sandbox's file system has the advantage of ensuring that the behavior of the script is correct, regardless of how it may be refactored or internally coded in the future. For instance, someone could remove the isNodeModulePath() function entirely in favor of a single inline RegExp, and this test would still pass (or appropriately fail if the RegExp is wrong), confirming that js_run_devserver's overall behavior is still correct.

Maybe in order to remove the need for a new attribute on js_run_devserver though (and to still not fully duplicate the entire macro in the test/ folder), we could do something like the following?

def js_run_devserver(
        name,
        **kwargs):
    """
    (Original macro docs with all args documented here)
    """
    js_run_devserver_internal(
        name = name,
        rule_to_execute = _js_run_devserver,
        **kwargs,
    )

def js_run_devserver_internal(
        name,
        rule_to_execute,
        tool = None,
        command = None,
        grant_sandbox_write_permissions = False,
        use_execroot_entry_point = True,
        allow_execroot_entry_point_with_no_copy_data_to_bin = False,
        **kwargs):

    rule_to_execute(
        name,
        # ... Original logic of js_run_devserver here with selects() and such ...
        **kwargs,
    )

That would leave the public API of js_run_devserver the same, but also provide a macro for the integration tests. Thoughts?

[gregjacobs]

Just pushed the above idea so you guys can take a look. Let me know what you think.

Btw, I would say not to focus on the fact that the particular fix in this PR is small - I think adding a test rule for js_run_devserver is good scaffolding where now other future integration tests can be written to confirm js_run_devserver's behavior. This would be an advantage in the future.

Or was your comment above more about just not using jasmine in the .spec.js file and having a "one test per Bazel target" kind of thing?

[gregjacobs]

Ok so I made the test target specific to just checking node_modules symlinks, but I kept Jasmine in because it provides matchers which have good error messages when things go wrong without needing to write if conditions + throw statements and such. It also provides a test description if say, the readLinkSync calls throw (would need to try/catch those to provide a good error message otherwise).

Lmk if that's okay to leave in, or if you really prefer it out (I'm okay either way :))

[gregmagolan]

🚀

[gregmagolan]

I had another read after your recent changes and I think your test here is great and very creatively wired up. Lets go with your approach. Like you said above, I think the pattern could be used in the future for more js_run_devserver assertions.

Looks like some CI failures and a buildifier failure to cleanup and this is good to go.

[gregjacobs]

Thanks for that @gregmagolan, and glad you like the approach! I fixed the buildifier errors, and I believe the CI failures as well.

I did have to add an interesting workaround inside js_run_devserver.bzl though for the rule_to_execute param of the internal macro. For some reason, if I added rule_to_execute as a named parameter, I'd get a weird error: "js_run_devserver_internal() got multiple values for parameter 'rule_to_execute'". Perhaps rule instances can't be passed to macros with named params?

What I did instead was pop the parameter off kwargs instead. See here: https://github.com/aspect-build/rules_js/pull/1210/files#diff-f43ca342bae64f71d41f4b9f371a8ff5b6eb168883d58438c35b41eaaa8b8403R285. Any insights on this would def be appreciated, although it does seem to work 🙂

[gregjacobs]

Awesome, looks like everything passed now!

[gregmagolan]

Awesome, looks like everything passed now!

Great! Thanks for greening it up. Before landing, I'll do some local testing do try to understand the rule_to_execute named parameter issue. That was a strange one.

[gregjacobs]

Yes, definitely a strange one. But awesome, sounds good! Let me know if you need any help recreating it. I was running the e2e/js_run_devserver on this commit and seeing the problem: 4c5326c

[gregmagolan]

Oh I see, you had rule_to_execute as a require 2nd parameter:

def js_run_devserver_internal(
        name,
        rule_to_execute,
        tool = None,
        command = None,
        grant_sandbox_write_permissions = False,
        use_execroot_entry_point = True,
        allow_execroot_entry_point_with_no_copy_data_to_bin = False,
        **kwargs):

So starlark must have taken that as tool at the callsite and then rule_to_execute = _js_run_devserver, ended up in kwargs as dup:

    js_run_devserver_internal(
        name,
        tool,
        command,
        grant_sandbox_write_permissions,
        use_execroot_entry_point,
        allow_execroot_entry_point_with_no_copy_data_to_bin,
        rule_to_execute = _js_run_devserver,
        **kwargs
    )

[gregmagolan]

The kwargs pop with the default as _js_run_devserver is much cleaner anyway

[gregmagolan]

Actually, with that pattern you don't even need an js_run_devserver_internal rule. It can be just a hidden implementation detail that you can override the rule_to_execute

[gregmagolan]

Pushed a commit to your branch that removes js_run_devserver_internal

[gregjacobs]

Good call on not needing js_run_devserver_internal at all with the hidden param. Thanks for cleaning that up! Looks great 👍

Oh I see, you had rule_to_execute as a require 2nd parameter
So starlark must have taken that as tool at the callsite and then rule_to_execute = _js_run_devserver, ended up in kwargs as dup

Omg I can't believe I missed that! I added positional arguments to the js_run_devserver_internal() call when they should have been named arguments 😂 Thanks for catching that, and for walking this PR in! Much appreciated.

Best,
Greg