Filter out webpacked workers before parsing the scripts in the ASBG #823
Conversation
|
💔 |
|
run CI |
|
💔 |
|
run ci |
|
💚 |
| if (currFile.substring(currFile.length() - 3, currFile.length()).equals(".js")) { | ||
| // Read __worker-chunks.json file containing a list of webpacked workers | ||
| // ignore worker scripts, so as to not attempt to generate bindings for them | ||
| if (filterWorkerFiles) { |
There was a problem hiding this comment.
We can skip this check. If we don't have a __worker-chunks.json file, the webpackWorkersExcludesList will be empty.
| logger.info("Task: traverseDirectory: Visiting JavaScript file: " + f.getName()) | ||
| inputJsFiles.add(f.getAbsolutePath()) | ||
| def currFile = f.getAbsolutePath(); | ||
| if (currFile.substring(currFile.length() - 3, currFile.length()).equals(".js")) { |
There was a problem hiding this comment.
We can extract that logic to a function. Sth like - isJsFile(fullPath)
| // Read __worker-chunks.json file containing a list of webpacked workers | ||
| // ignore worker scripts, so as to not attempt to generate bindings for them | ||
| if (filterWorkerFiles) { | ||
| if (webpackWorkersExcludesList.any{element -> file(element).getAbsolutePath() == currFile}) { |
There was a problem hiding this comment.
We can move that to a separate function - isWorkerScript(file)
| logger.info("Task: traverseDirectory: Visiting JavaScript file: " + f.getName()) | ||
| inputJsFiles.add(f.getAbsolutePath()) | ||
| def currFile = f.getAbsolutePath(); | ||
| if (currFile.substring(currFile.length() - 3, currFile.length()).equals(".js")) { |
There was a problem hiding this comment.
Also, we can move the check that's below here -
if (isJsFile(currFile) && !isWorkerScript(currFile)) {
// ...
}
| if (workersExcludeFile.exists()) { | ||
| filterWorkerFiles = true; | ||
| webpackWorkersExcludesList = new JsonSlurper().parseText(workersExcludeFile.text) | ||
| } |
There was a problem hiding this comment.
if the workersExcludeFile doesn't exist, please add some kind of warning or error
There was a problem hiding this comment.
Why? The app may be built without webpack, or without the worker-loader if the user decided to remove it manually.
There was a problem hiding this comment.
Because if it's an app with webpack and the file does not exist, a warning can go a long way towards debugging the problem. In cases, users are dealing with webpack. I'm sure there's a way to add a check if the project is a webpack one and I believe it's important.
If you believe we shouldn't do it, don't, that why I left a comment not a request for change.
|
💚 |
| def isWorkerScript = { fileName -> | ||
| // Read __worker-chunks.json file containing a list of webpacked workers | ||
| // ignore worker scripts, so as to not attempt to generate bindings for them | ||
| if (webpackWorkersExcludesList.any{element -> file(element).getAbsolutePath() == fileName}) { |
There was a problem hiding this comment.
Shouldn't that be just
return webpackWorkersExcludesList.any{element -> file(element).getAbsolutePath() == fileName}
?
There was a problem hiding this comment.
That's right, I have unintentionally left the if statement!
petekanev
force-pushed
the
pete/ignore-parsing-webpacked-workers
branch
from
243ef35 to
ab356c8
Compare
|
💚 |
|
@Plamen5kov thanks for your input. Please note that besides the list of workers to exclude from parsing, Gradle doesn't know if its building a webpacked, or a regular project, thus adding a warning message on every build which doesn't involve webpacking, I think will confuse users more than it would help others who webpack. |
4 participants
Problem:
As explained in #778 - webpack will bundle essential code inside the worker chunks, and that will cause the static binding generator's parser to traverse and generate the same classes as many times as there are workers. The logic in the ASBG prevents a class with the same name from being generated more than once to guarantee predictable runtime behavior, and will crash build-time.
Proposed Solution:
Read a list of the worker scripts output by the webpack worker loader, and exclude the script chunks from being javascript-parsed. Should there be extending of classes the runtime binding generator will take care of handling it the first time per app installation.
Depends on a custom implementation of the worker-loader, and existing webpack user configurations importing the custom plugin.
Addresses: #778