feat(pwa-kit-dev): improve dependency resolving for `findInProjectThenSDK` #978

ecRobertEngel · 2023-02-10T11:38:36Z

Description

This PR tries to improve dependency resolving of the webpack configuration. I also have some questions and I am looking for your feedback for that.

Why this change
The current logic of findInProjectThanSdk has a few problems:

It does not support a mono-repository setup (issue mentioned here: type error when using npm workspaces monorepo setup #869 and [BUG] findInProjectThenSDK doesn't resolve correct path #765). This will probably be a common setup as many companies are using mono-repositories. Both pwa-kit projects I am supporting are using such a set up and the issuer seems to plan to do so aswell.
The current logic does not really resolve some packages. The ternary of the current logic does not check again if the dependency really exists. It just assumes that it is here which is often times untrue. The packages babel-runtime @tanstack/react-query and html-loader are all not resolved by this logic. This can cause very confusing behavior.

Solution Approach
I tried to make the solution both more flexible and robust:

I added an optional third-level search for common mono-repository setups to resolve packages installed at the project root. This still fully supports the logic to first check project and sdk first and only fallsback if it could not be resolved there. I also made this check optional so that it will not cause issues for non mono-repository setups by using an environment variable.
I added a check to explicitly check if a dependency could be resolved at each level. If a dependency could not be found at all I added an error and abort the build

Next steps
The PR in it's current stage will not build a project successfully. This is because the added error throws if a dependency could not be resolved. In my opinion this should be the correct behavior as everything else will just cause issues down the line. But please provide me some feedback on this. In the meantime I think it makes sense to check why these dependencies are not resolved and how to move forward:

babel-runtime
@tanstack/react-query
html-loader

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

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

(change1)

How to Test-Drive This PR

(step1)

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

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

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

ecRobertEngel · 2023-02-10T11:42:36Z

Hacky way to test drive the mono repository logic: install babel-runtime at the root level of the project and run a build with MONOREPO=true - the missing dependency should now be resolved.

wjhsf · 2023-02-10T18:33:33Z

packages/pwa-kit-dev/src/configs/webpack/config.js

+ * Find the path to a dependency
+ * First check the project's node_modules, then the SDK's node_modules, then the monorepo's node_modules


I don't know the history of this method, but it seems like the goal is "If node's module resolution can't find the package, check this other location as a fallback." To that end, why not just leverage node's built-ins? Something like

try { return require.resolve(pkg) } catch (err) { const sdkPath = resolve(sdkDir, 'node_modules', pkg) if (fs.existsSync(sdkPath)) return sdkPath else throw err }

+1. TIL require.resolve. Here's an article that talks about it: https://dev.to/stephencweiss/what-is-require-resolve-and-how-does-it-work-1ho4

Late to the party. But I think there is an opportunity to simplify things even more.

Going back, the history of that function was to solve a problem we created for ourselves by using a lerna mono repo that doesn't use it's main feature, hoisting dependencies (I believe that there was a reason why we chose not to hoist, but I can't remember).

Because of the decision not to hoist we continually ran into problems where modules that have global context like react-helmet or react-loadable didn't work 100% because the sdk was using one instance of the module and the pwa template was using another. So to solve this we used webpack alias's and that funky little utility called findInProjectThenSDK, at the time we didn't think of using resolve as we know where the dependencies were. So we can make things better by using that as you suggest @wjhsf, but what if we did something a little more daring....

What if we decided to start hoisting our deps? or replace lerna with npm workspaces. Doing this would solve the root problem so we didn't need special functions to find modules, internally the npm module resolution would kinda just work 🤞 Sounds simple, probably isn't going to be. But I think it's better than what we have now.

I did a proof of concept where I generated and placed it in a npm workspace project and ripped out the findInProjectThenSDK and a few other minor changes and was able to get things working. So there is some promise.

@bendvc Template Extensibility uses findInProjectThenInSDK (and modifies it). Can you share what you were working on so I can make sure it works for the new feature?

vmarta

Thanks @ecRobertEngel for this PR. It does reveal a few bugs with our current implementation and also a feature request to support monorepo too.

vmarta · 2023-02-14T00:18:57Z

packages/pwa-kit-dev/src/configs/webpack/config.js

+    if (fs.existsSync(sdkPath)) {
+        return sdkPath
+    }
+    if (isMonoRepo) {


As you mentioned in your PR description, I don't think we have considered the scenario of developers having their own monorepo. So I'd consider this to be a new feature.

However, there is a bug with the current implementation: for those developers who do not have a monorepo, the sdkDir is actually not resolved correctly.

pwa-kit/packages/pwa-kit-dev/src/configs/webpack/config.js

Line 26 in 24b8b16

const sdkDir = resolve(path.join(__dirname, '..', '..', '..'))

The above line would resolve correctly only for those folks developing PWA Kit:

Within PWA Kit monorepo: packages/pwa-kit-dev/src/configs/webpack/config.js → sdkDir is mapped to packages/pwa-kit-dev/

Within a generated project (and no monorepo): node_modules/pwa-kit-dev/configs/webpack/config.js → sdkDir is mapped to node_modules (and not node_modules/pwa-kit-dev/)

vmarta · 2023-02-14T00:24:23Z

packages/pwa-kit-dev/src/configs/webpack/config.js

+            return monoRepoPath
+        }
+    }
+    throw new Error(`Could not find dependency ${pkg}`)


You mentioned a few dependencies that don't seem to exist anywhere. One of them is react-query. It actually can be found in pwa-kit-react-sdk package, but not the pwa-kit-dev.

Again, I think there's another bug with sdkDir. It equates 'sdk' to mean pwa-kit-dev, where in fact we may want to check the other pwa-kit-related packages too. I think this is a regression when we moved to V2 of PWA Kit. In V1, there used to be only pwa-kit-react-sdk, but in V2 we split it into smaller packages.

vmarta · 2023-02-14T00:27:31Z

packages/pwa-kit-dev/src/configs/webpack/config.js

+ * @param {string} pkg - name of the dependency
+ * @returns {string} path to the dependency
+ */
+const findDependency = (pkg) => {
    const projectPath = resolve(projectDir, 'node_modules', pkg)


I think to be aligned with the original intention (find in project first and then in the sdk), I would check isMonoRepo earlier in the code block.

Suggested change

const projectPath = resolve(projectDir, 'node_modules', pkg)

const projectPath = isMonoRepo ? resolve(projectDir, '..', '..', 'node_modules', pkg) : resolve(projectDir, 'node_modules', pkg)

git2gus · 2023-02-14T00:36:28Z

This issue has been linked to a new work item: W-12539749

bfeister · 2023-04-17T13:09:29Z

@ecRobertEngel any update on this? We've labeled this a regression and are tracking internally. Need to know the status

ecRobertEngel · 2023-05-02T19:59:26Z

@bfeister we have replace the build logic for this completely as it did not fit our setup at all. I would need to setup a new project to see the current status of this issue. I did not track the changes that are done as part of the the extensibility poc and if they can resolve this.

feat(pwa-kit-dev): improve dependency resolving

36a13de

ecRobertEngel requested a review from a team as a code owner February 10, 2023 11:38

wjhsf reviewed Feb 10, 2023

View reviewed changes

vmarta reviewed Feb 14, 2023

View reviewed changes

vmarta changed the title ~~feat(pwa-kit-dev): improve dependency resolving~~ feat(pwa-kit-dev): improve dependency resolving for findInProjectThenSDK Feb 14, 2023

vmarta added the BUG P1 git2gus Label regression label Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pwa-kit-dev): improve dependency resolving for `findInProjectThenSDK` #978

feat(pwa-kit-dev): improve dependency resolving for `findInProjectThenSDK` #978

ecRobertEngel commented Feb 10, 2023 •

edited

ecRobertEngel commented Feb 10, 2023

wjhsf Feb 10, 2023

vmarta Feb 13, 2023

bendvc Mar 11, 2023

bendvc Mar 11, 2023

bfeister Mar 23, 2023 •

edited

vmarta left a comment

vmarta Feb 14, 2023 •

edited

vmarta Feb 14, 2023

vmarta Feb 14, 2023 •

edited

git2gus bot commented Feb 14, 2023

bfeister commented Apr 17, 2023

ecRobertEngel commented May 2, 2023

		* Find the path to a dependency
		* First check the project's node_modules, then the SDK's node_modules, then the monorepo's node_modules

	const projectPath = resolve(projectDir, 'node_modules', pkg)
	const projectPath = isMonoRepo ? resolve(projectDir, '..', '..', 'node_modules', pkg) : resolve(projectDir, 'node_modules', pkg)

feat(pwa-kit-dev): improve dependency resolving for findInProjectThenSDK #978

Are you sure you want to change the base?

feat(pwa-kit-dev): improve dependency resolving for findInProjectThenSDK #978

Conversation

ecRobertEngel commented Feb 10, 2023 • edited

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

ecRobertEngel commented Feb 10, 2023

wjhsf Feb 10, 2023

Choose a reason for hiding this comment

vmarta Feb 13, 2023

Choose a reason for hiding this comment

bendvc Mar 11, 2023

Choose a reason for hiding this comment

bendvc Mar 11, 2023

Choose a reason for hiding this comment

bfeister Mar 23, 2023 • edited

Choose a reason for hiding this comment

vmarta left a comment

Choose a reason for hiding this comment

vmarta Feb 14, 2023 • edited

Choose a reason for hiding this comment

vmarta Feb 14, 2023

Choose a reason for hiding this comment

vmarta Feb 14, 2023 • edited

Choose a reason for hiding this comment

git2gus bot commented Feb 14, 2023

bfeister commented Apr 17, 2023

ecRobertEngel commented May 2, 2023

feat(pwa-kit-dev): improve dependency resolving for `findInProjectThenSDK` #978

feat(pwa-kit-dev): improve dependency resolving for `findInProjectThenSDK` #978

ecRobertEngel commented Feb 10, 2023 •

edited

bfeister Mar 23, 2023 •

edited

vmarta Feb 14, 2023 •

edited

vmarta Feb 14, 2023 •

edited