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.

Sign up for GitHub

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

feat(pwa-kit-dev): minor performance improvements and added comments #974

Merged
merged 6 commits into from
Mar 23, 2023
Merged

feat(pwa-kit-dev): minor performance improvements and added comments #974

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bd778a0
feat(pwa-kit-dev): minor performance improvements and added comments
ecRobertEngel Feb 9, 2023
d5912fb
docs(pwa-kit-dev): clean up comments
ecRobertEngel Feb 14, 2023
0eef59f
refactor(pwa-kit-dev): update condition from PR feedback
ecRobertEngel Feb 14, 2023
ae27f75
Merge branch 'develop' into feature/minor-webpack-plugin-improvements
ecRobertEngel Feb 15, 2023
1d7a239
Merge branch 'develop' into feature/minor-webpack-plugin-improvements
wjhsf Mar 22, 2023
5fc0d66
Merge branch 'develop' into feature/minor-webpack-plugin-improvements
wjhsf Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions packages/pwa-kit-dev/src/configs/webpack/plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,18 @@ import fs from 'fs'
* @returns {webpack.NormalModuleReplacementPlugin}
*/
export const createModuleReplacementPlugin = (projectDir) => {
// Helper function to create a RegExp object from a string
const makeRegExp = (str, sep = path.sep) => {
// Replace unix paths with windows if needed and build a RegExp
if (sep === '\\') {
str = str.replace(/\//g, '\\\\')
}
return new RegExp(str)
}

// List of overridable paths
// path: The RegExp that matches the path to the overridable component
// newPath: The path to the component in the project directory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's not always a component...

Suggested change
// path: The RegExp that matches the path to the overridable component
// newPath: The path to the component in the project directory
// path: The RegExp that matches the path to the overridable file
// newPath: The corresponding path in the project directory

const overridables = [
{
path: makeRegExp('pwa-kit-react-sdk(/dist)?/ssr/universal/components/_app-config$'),
Expand All @@ -48,6 +53,7 @@ export const createModuleReplacementPlugin = (projectDir) => {
]
const extensions = ['.ts', '.tsx', '.js', '.jsx']

// Find the replacement for each overridable path by checking if the file exists
const replacements = []
overridables.forEach(({path, newPath}) => {
extensions.forEach((ext) => {
Expand All @@ -58,20 +64,30 @@ export const createModuleReplacementPlugin = (projectDir) => {
})
})

// Return a new `webpack.NormalModuleReplacementPlugin` instance
return new webpack.NormalModuleReplacementPlugin(/.*/, (resource) => {
const resolved = path.resolve(resource.context, resource.request)
// We only want to replace resources that are requested from the SDK
if (resource.context.includes('pwa-kit-react-sdk')) {
// Resolve the full path of the resource
const resolved = path.resolve(resource.context, resource.request)

const replacement = replacements.find(({path}) => resolved.match(path))
// Find the replacement for the resolved path from the overridables list
const replacement = replacements.find(({path}) => resolved.match(path))

const sdkPaths = [
path.join('packages', 'pwa-kit-react-sdk'),
path.join('node_modules', 'pwa-kit-react-sdk')
]
if (replacement) {
// Check if the resource was requested from 'packages/pwa-kit-react-sdk' or 'node_modules/pwa-kit-react-sdk'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment is redundant if we repeat the paths. In the future, if there are changes to the paths, the comment may be forgotten to be updated.

Sometimes it's better to comment the why, rather than what the the line of code actually does.

Suggested change
// Check if the resource was requested from 'packages/pwa-kit-react-sdk' or 'node_modules/pwa-kit-react-sdk'
// Check if the resource was requested from these paths
// The SDK can appear in different locations depending if you're developing pwa-kit or working in your own project

const sdkPaths = [
path.join('packages', 'pwa-kit-react-sdk'),
path.join('node_modules', 'pwa-kit-react-sdk')
]

const requestedFromSDK = sdkPaths.some((p) => resource.context.includes(p))
const requestedFromSDK = sdkPaths.some((p) => resource.context.includes(p))

if (requestedFromSDK && replacement) {
resource.request = replacement.newPath
// If the resource was requested from SDK, replace the resource request with the replacement path
if (requestedFromSDK) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving this check earlier in the logic? I think conceptually it goes together with the first if condition.

So I'm thinking something like this:

if (resource.context.includes('pwa-kit-react-sdk') && sdkPaths.some((p) => resource.context.includes(p))) {
  ...
}

That (A && B) condition still follows your intention of short-circuiting when resource.context does not include "pwa-kit-react-sdk".

resource.request = replacement.newPath
}
}
}
})
}