SalesforceCommerceCloud / pwa-kit

React-based JavaScript frontend framework to create a progressive web app (PWA) storefront for Salesforce B2C Commerce.
https://developer.salesforce.com/docs/commerce/pwa-kit-managed-runtime/guide/pwa-kit-overview.html
BSD 3-Clause "New" or "Revised" License
276 stars 126 forks source link

Fix: 'Cannot use import statement outside a module' error in generated extensible project unit tests @W-15705195@ #1821

Closed joeluong-sfcc closed 1 month ago

joeluong-sfcc commented 1 month ago

Description

There is currently an issue (https://github.com/SalesforceCommerceCloud/pwa-kit/issues/1763) in generated projects with extensibility where if you try to run unit tests, you can end up with the following error:

Jest encountered an unexpected token

...

Details:

    /retail-react-app-demo/node_modules/@salesforce/retail-react-app/app/components/_app-config/index.jsx:16

import React from 'react'
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      1 | import React from 'react'
      2 | import {render, waitFor} from '@testing-library/react'
    > 3 | import AppConfig from '@salesforce/retail-react-app/app/components/_app-config/index.jsx'
        | ^

Note that the SyntaxError: Cannot use import statement outside a module error comes from within the node_modules, specifically at node_modules/@salesforce/retail-react-app/app/components/_app-config/index.jsx

This issue occurs because transformations are not applied to node_modules by default. The files within the node_modules/@salesforce/retail-react-app directory are not formatted in the way that Jest expects. The fix is to ensure transformations are applied to node_modules/@salesforce/retail-react-app by configuring it in the jest.config.js file.

Steps to reproduce:

  1. Generate project: npx @salesforce/pwa-kit-create-app@latest --preset=retail-react-app-demo
  2. cd retail-react-app-demo
  3. Add a unit test (index.test.js) to retail-react-app-demo/overrides/app/components/_app-config/
// Import a variety of things from the node_modules
import React from 'react'
import {render, waitFor} from '@testing-library/react'

import AppConfig from '@salesforce/retail-react-app/app/components/_app-config/index.jsx'

import {CorrelationIdProvider} from '@salesforce/pwa-kit-react-sdk/ssr/universal/contexts'
import {uuidv4} from '@salesforce/pwa-kit-react-sdk/utils/uuidv4.client'
import {StaticRouter} from 'react-router-dom'

import mockConfig from '@salesforce/retail-react-app/config/mocks/default'
import {rest} from 'msw'
import {registerUserToken} from '@salesforce/retail-react-app/app/utils/test-utils'

test('Sample unit test', () => {
    expect(true).toBe(true)
})
  1. npm run test
  2. Observe failure

Types of Changes

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

How to Test-Drive This PR

  1. git checkout ju/fix-import-extensible-tests
  2. From the root directory: node ./packages/pwa-kit-create-app/scripts/create-mobify-app.js --preset=retail-react-app-demo
  3. cd retail-react-app-demo
  4. Add sample unit test from above (index.test.js) to retail-react-app-demo/overrides/app/components/_app-config/
  5. npm run test
  6. Observe no failures

Checklists

General

Accessibility Compliance

You must check off all items in one of the follow two lists:

or...

Localization

bendvc commented 1 month ago

@joeluong-sfcc I think we need to think about the bigger picture here, does it make sense for us to allow end-users the ability to run tests for projects that we are extending from? Have we thought about ignoring running any tests that are in the extended project as a solution here?

joeluong-sfcc commented 1 month ago

@joeluong-sfcc I think we need to think about the bigger picture here, does it make sense for us to allow end-users the ability to run tests for projects that we are extending from? Have we thought about ignoring running any tests that are in the extended project as a solution here?

@bendvc You bring up a good point. I think that certain tests wouldn't really make sense in an extended project, like tests that retest the implementation of a component from the underlying project. However, I still think we should allow end users to write unit tests in their extended project using exported components/functions from the underlying project. For example, I can imagine a scenario where a user would want to test the behavior of their overriding component and compare it to the underlying component that they're overriding.

I'd like to get your thoughts as well, do you think it makes sense to allow users to write tests in this manner (or other tests in general) in their extended projects?

bendvc commented 1 month ago

@joeluong-sfcc I think we need to think about the bigger picture here, does it make sense for us to allow end-users the ability to run tests for projects that we are extending from? Have we thought about ignoring running any tests that are in the extended project as a solution here?

@bendvc You bring up a good point. I think that certain tests wouldn't really make sense in an extended project, like tests that retest the implementation of a component from the underlying project. However, I still think we should allow end users to write unit tests in their extended project using exported components/functions from the underlying project. For example, I can imagine a scenario where a user would want to test the behavior of their overriding component and compare it to the underlying component that they're overriding.

I'd like to get your thoughts as well, do you think it makes sense to allow users to write tests in this manner (or other tests in general) in their extended projects?

Yes. I agree that if we have a "test" script it implies that we have some kind of testing framework setup and executing that script will run it. So if a developer that is working on an extended project wants to add tests for a new component, whether that component is a new page or an override, it should be executed when they use npm run test. But what shouldn't happen is the entire set of tests being run for the underlaying template that is being extended.

I'm open to discussing reasons why we might want to do that, but I can't think of any off the top of my head. The template being extended is, kind of a "grey" box, we know the api and how to use it, but we shouldn't be running the tests we created during an extended projects build cycle.