Implement page copying into generated from original theme (in case you starting with existing app)#7
Conversation
…u starting with existing app)
yeegor
left a comment
yeegor
left a comment
There was a problem hiding this comment.
Overall, the functionality seems good, although some modifications are required logc-wise and coding style-wise. Please, provide them, then we'll do a CR again!
Additional comment: please, make sure that require.resolve works OK in the modules with these changed imports. It seems that it currently is not handled, although on server side it sometimes is used.
| "name": "@scandipwa/nextjs-scripts", | ||
| "description": "Scripts and configuration used by CSA.", | ||
| "version": "0.0.0", | ||
| "bin": { | ||
| "nextjs-scripts": "./bin/nextjs-scripts.js" | ||
| }, | ||
| "engines": { | ||
| "node": ">=12.0.0" | ||
| }, | ||
| "dependencies": { | ||
| "@scandipwa/scandipwa-dev-utils": "^0.0.20", | ||
| "chokidar": "^3.5.1", | ||
| "cross-spawn": "^7.0.3", | ||
| "debounce": "^1.2.0", | ||
| "glob": "^7.1.6", | ||
| "next": "10.0.5", | ||
| "tree-kill": "^1.2.2", | ||
| "@babel/preset-react": "^7.12.13" | ||
| }, | ||
| "license": "OSL-3.0", | ||
| "publishConfig": { | ||
| "access": "public" | ||
| } |
There was a problem hiding this comment.
Pls keep this intact if you did not modify actual contents. Same as one of the comments above.
| } = parts.shift(); | ||
|
|
||
| result.push(source.substring(lastIndex, start)); | ||
| const code = source.substring(start, end); |
There was a problem hiding this comment.
code is not the most verbose name for this. Maybe importSource?
source is also code. Please, rename this :)
| code.replace(/[`'"]([^`'"]+)[`'"]/, (match, value) => { | ||
| const resolvedModulePath = tranformer(value); | ||
|
|
||
| return `"${resolvedModulePath}"`; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
If you have the sources' values as described above, you will be able to just cut away first and last symbol to get rid of the quote marks, instead of matching them by regex. That could be a tiny bit faster, and work more stable.
yeegor
left a comment
yeegor
left a comment
There was a problem hiding this comment.
The functionality seems valid.
Although, mutating external data within array methods is considered bad practice. Please see the comments.
| .forEach(([sourceFile, | ||
| targetFile]) => { |
There was a problem hiding this comment.
Splitting this into 2 lines is against our coding style. Please make this oneline.
| importSource.replace(/[`'"]([^`'"]+)[`'"]/, (match, value) => { | ||
| const resolvedModulePath = transformer(value); | ||
|
|
||
| return `"${ resolvedModulePath }"`; | ||
| }) |
There was a problem hiding this comment.
Still, CallExpression nodes are used instead of sources within the nodes. Why so? See the previous CR for details. This would free you of this regexp.
2 participants
Implement page copying into generated from original theme (in case you starting with existing app)