Detect/resolve ambiguous script names

[codersmith]

Detects and resolves ambiguous script name combinations like [foo.js, foo.ts] or [foo.ts, foo.json] when running yarn rw exec foo.

Fixes #9249

Problem

#9249 identified that when you have a script file called scripts/foo.ts along with a JSON data file with the same name scripts/foo.json, that trying to run yarn rw exec foo breaks.

The problem is that when calling require('foo'), require finds the JSON file first and loads that vs. loading the .ts file.

Solution

There are two parts to the fix:

1. Resolve any ambiguity ahead of trying to load and run the script.
   For example, if the script name is fully qualified e.g. yarn rw exec foo.ts, then that should work.
   But if two scripts exist such as [foo.js, foo.ts], and the command is yarn rw exec foo, then this is ambiguous and a nice error message should be displayed to help the user clarify their intent.

2. Any non-script files (like JSON files, etc.) should not conflict when resolving a potential script file. Script files are identified with extensions .{js,jsx,ts,tsx}
   To do this, we can resolve foo into foo.ts prior to attempt loading it with require.

---

EDIT (by @Tobbe):

Example output:

Normally it'll just list the names of the scripts. But if there is a conflict between names (two or more files with the same name, only different file extensions) then the file extension is included in the output

.json files (or other files that we can't execute) are not included in the list of scripts

Running a script by just the name prints an error if there's ambiguity

And as for testing, unfortunately we can't unit test actually running the script. The issue is with CJS/ESM (require vs import) and vitest. So for now only the listing of scripts is tested. Running the scripts has only been tested manually.

[codersmith]

Hi there, I'm looking for some early feedback on this PR as well as pointers to where any relevant tests may be located.

I described my basic thought process here: #9249 (comment)

Thanks!

[Tobbe]

Thanks a lot for your PR! You're off to a great start 🙂

Looks like there are no existing tests for this code. Would be great if you could add some for sure!

[codersmith]

Looks like there are no existing tests for this code. Would be great if you could add some for sure!

Happy to. I'll need to modify the test project state to simulate different combinations of script files existing. Can you point me to where those types of preconditions might be established, or an example test that does something similar?

[Tobbe]

Looks like there are no existing tests for this code. Would be great if you could add some for sure!

Happy to. I'll need to modify the test project state to simulate different combinations of script files existing. Can you point me to where those types of preconditions might be established, or an example test that does something similar?

I'd lean on mocking instead of using the test project, if possible. We've started using memfs in some unit tests lately, and it's pretty nice to work with 🙂
So search for memfs in our codebase and take a look at those test - see if that gives you any ideas. If not - just let me know and I'll try to write one test to get you started

[codersmith]

Examples of error messages. Note that previously the error message did not include the file extension in the available scripts.

$ yarn rw exec fooDoesNotExist.ts

No script called fooDoesNotExist.ts in ./scripts folder.

Available scripts:
- foo.js
- foo.ts
- seed.js

$ yarn rw exec fooDoesNotExist

No script called fooDoesNotExist.{js,jsx,ts,tsx} in ./scripts folder.

Available scripts:
- foo.js
- foo.ts
- seed.js

$ yarn rw exec foo

Multiple scripts called foo.{js,jsx,ts,tsx} in ./scripts folder.

Available scripts:
- foo.js
- foo.ts
- seed.js

And case when no script name is specified still works:

$ yarn rw exec
Available scripts:
- foo.js
- foo.ts
- seed.js

Examples of valid executions:
yarn rw exec foo when there is only one foo.{js,jsx,ts,tsx} file
yarn rw exec foo.js when foo.js exists
yarn rw exec foo.js when [foo.js, foo.json] exist
yarn rw exec foo.ts when [foo.ts, foo.json] exist (NOTE: this case was the originally identified bug)
yarn rw exec foo.js when [foo.js, foo.ts] exist

[Tobbe]

Examples of error messages.

Those are all awesome!

[codersmith]

Those are all awesome!

Thanks! It's also worth pointing out that I think the only reason we can't just use the same resolveSourcePath function to resolve the file name ahead of execution is because of the detail we need to produce helpful error messages.

    console.error(
      c.error(
        `\n${
          !targetMatch.length ? 'No script' : 'Multiple scripts'
        } called ${c.underline(
          targetScriptPath.ext
            ? targetScriptPath.base
            : targetScriptPath.name + '.{js,jsx,ts,tsx}'
        )} in ./scripts folder.\n`
      )
    )

[codersmith]

I'd lean on mocking instead of using the test project, if possible. We've started using memfs in some unit tests lately, and it's pretty nice to work with 🙂 So search for memfs in our codebase and take a look at those test - see if that gives you any ideas. If not - just let me know and I'll try to write one test to get you started

Thanks for the pointer. I was able to put together a test that works for a basic case. Mind taking a look and letting me know if I'm on the right track?

Thanks!

//
// - execHandler.test.js
//
import { vol } from 'memfs'

import { runScriptFunction as mockRunScriptFunction } from '../../lib/exec'
import { handler } from '../execHandler'

jest.mock('fs', () => require('memfs').fs)
jest.mock('../../lib/exec')

const redwoodProjectPath = '/redwood-app'
process.env.RWJS_CWD = redwoodProjectPath

describe('execHandler', () => {
  afterEach(() => {
    vol.reset()
    mockRunScriptFunction.mockRestore()
  })

  describe('handler', () => {
    it('verify exec script', async () => {
      const mockExit = jest.spyOn(process, 'exit').mockImplementation()

      const fooScript =
        'export default async ({ args }) => { console.log(`:: Executing script ${__filename} ::`) }'

      vol.fromNestedJSON(
        {
          'redwood.toml': '',
          scripts: {
            'foo.js': fooScript,
          },
        },
        redwoodProjectPath
      )

      await handler({ name: 'foo', arg1: 'a1', arg2: 'a2' })

      expect(mockExit).not.toHaveBeenCalled()
      expect(mockRunScriptFunction).toHaveBeenCalledWith({
        path: '/redwood-app/scripts/foo',
        functionName: 'default',
        args: { args: { arg1: 'a1', arg2: 'a2' } },
      })

      mockExit.mockRestore()
    })
  })
})

[Tobbe]

A few comments on the test:

• Maybe mock generatePrismaClient to speed things up (if it makes a difference)
• I'd like a more descriptive test case name. Maybe it('runs .js scripts without providing an extension'
• I think mockExit.mockRestore() is never called if the test fails. If I'm right the solution is to move all mock setup and teardown to beforeEach/beforeAll and afterEach/afterAll
• I wouldn't mock ../lib/exec because then you're not testing the change you made to that file. I think a better option here is to just spy on it, so that you can still verify input parameters.
• Verify the return value of runScriptFunction. That way you can know that the script was properly executed (just have the script return a value that's easy to verify, like a specific number or something

[codersmith]

Thanks for the detailed guidance! I'll address your points as I get together the rest of the test cases, hopefully wrapping up later today.

Quick question regarding this one:

I wouldn't mock ../lib/exec because then you're not testing the change you made to that file. I think a better option here is to just spy on it, so that you can still verify input parameters.

I was planning to write separate tests focused on testing the changes to exec.js and files.js vs. the one here which are focused on execHandler.js. So in this test I was just making sure that runScriptFunction is called, and in the exec tests I'll check that the script actually runs. Does that sound reasonable?

[codersmith]

I am not sure how to test running the actual script, at least not when using memfs.

The code in question calls require('/redwood-app/scripts/foo.js'), but this fails to load the test script module, probably because the test process is running within the context of the actual framework directory (e.g. '/Users/user/dev/redwoodjs/redwood'), not from the imaginary 'memfs' file system.

[codersmith]

Hi @Tobbe, in this case should I look to modify/add an integration test?

[Tobbe]

I found https://github.com/streamich/fs-monkey/blob/master/docs/api/patchRequire.md to help with the require problem 🙂 Take a look in the tests I pushed

[Tobbe]

Sorry I went radio silent on this one @codersmith 📻
We're pushing hard to get a new stable release of RW out the door, so I just got too busy.

I'll try to give it some thought before the end of the week

[codersmith]

No problem, understand. Thanks for the vitest updates

[Tobbe]

@codersmith I added a few tests. I'm not super happy with them. But mostly because the way patchRequire seems to work. I wish there was a way to reset and/or restore. But I can't find anything. At at the very least clear internal require cache. But nothing turned up when I googled.

Also left a TODO comment in the test that I'd like you to take a look at, and see if you can come up with something better.

[codersmith]

Also left a TODO comment in the test that I'd like you to take a look at, and see if you can come up with something better.

Just to be clear, are you referring to being able to write the test script using module vs common syntax like:

        export default async ({ args }) => ${expectedScriptReturn}

(fails with "Module /redwood-app/scripts/fooModule.js:2 seems to be an ES Module but shipped in a CommonJS package.")

vs

        module.exports = {
          default: ({ args }) => ${expectedScriptReturn}
        }

(succeeds, but isn't typical syntax for RW scripts.)

One thing that came to mind was to switch the script runner from using require to dynamic import:

  // old
  const script = require(resolveSourcePath(scriptPath))
  const returnValue = await script[functionName](args)
-------
  // new
  const returnValue = await import(resolveSourcePath(scriptPath)).then(
    ({ default: script }) => script[functionName](args)
  )

But that gets pretty tricky, as now we can't use patchRequire. We could arguably use vi.mock to mock the import of the test script package, but that syntax (which I haven't quite got working yet) ends up looking quite a bit different than how a user would write the script anyway, which is what we're trying to avoid. It also has some complexity due to vi.mock being hoisted.

[Tobbe]

(fails with "Module /redwood-app/scripts/fooModule.js:2 seems to be an ES Module but shipped in a CommonJS package.")

I got that message too 😄

What happens when you start testing with ts scripts? /scripts/fooTS.ts I don't even know if patchRequire supports that. So you might have to get creative!

[codersmith]

@Tobbe, I'm just getting back to this PR, and noticed that after the tests were switched from jest to vitest that they're not working.

I could use some help figuring out the issue, which I think has to do with setting up mocks for project-config based on what I'm seeing other tests do, but I haven't been able to get it working.

For reference, I temporarily created two different implementations of the same test case:
execHandler-vitest.test.js
execHandler-jest.test.js

The jest test works, but I'm getting an error with vitest: "Could not find a "redwood.toml" file, are you sure you're in a Redwood project?"

Is this something you've seen before, or do you have an ideas on what changes I should make to get the vitest version running?

[codersmith]

I've tried a few different variations of declaring this mock, such these, but I get the same error.

vi.mock('@redwoodjs/project-config', async (importOriginal) => {
  const path = require('path')
  const originalProjectConfig = await importOriginal()
  const BASE_PATH = '/redwood-app'
  return {
    ...originalProjectConfig,
    getPaths: () => {
      return {
        base: BASE_PATH,
        scripts: path.join(BASE_PATH, 'scripts'),
      }
    },
  }
})

and

vi.mock('@redwoodjs/project-config', () => {
  const BASE_PATH = '/redwood-app'
  return {
    resolveFile: (path) => path,
    getPaths: () => {
      return {
        base: BASE_PATH,
        scripts: path.join(BASE_PATH, 'scripts'),
      }
    },
    getConfigPath: () => {
      return 'redwood.toml'
    },
    getConfig: () => {
      return {}
    },
  }
})

[Josh-Walker-GM]

Looks great. Tested locally too and it works nicely.