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.

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

parser.hooks.new not called for classes within the same file. #18113

Open
Danielku15 opened this issue Feb 23, 2024 · 7 comments · May be fixed by #18128
Open

parser.hooks.new not called for classes within the same file. #18113

Danielku15 opened this issue Feb 23, 2024 · 7 comments · May be fixed by #18128

Comments

@Danielku15
Copy link

Bug report

What is the current behavior?

The parser.hooks.new("Item") hooks are not called when the class Item is within the same file/module. It works if the class Item is imported from another module.

If the current behavior is a bug, please provide the steps to reproduce.

  1. Use this repository: https://github.com/Danielku15/webpack-parser-hook-call
  2. Run npm install && npm run build
  3. Notice in the console output that the hook is not called for new NotWorkingClass

In the webpack.config.js you will find a small plugin tapping into the parser.hooks.new for ESM javascript files. It then expects to be called for all the new expressions within input.mjs. But there is no call for NotWorkingClass

What is the expected behavior?

The parser hook should also be called for the new NotWorkingClass expression. From what I could see is following path prevents the hook to be called:

  • JavaScriptParser.walkNewExpression is called for all expressions which calls JavaScriptParser.callHooksForExpression
  • For WorkingClass and Uint8Array the JavaScriptParser.getMemberExpressionInfo returns some data which allows the right hooks to be called.
  • For NotWorkingClass undefined is returned. This is because in JavaScriptParser.getFreeInfoFromVariable the freeName is set to true instead of a string bailing the whole code path:
    if (typeof name !== "string") return undefined;

I'm not sure what getFreeInfoFromVariable should return in this case from a low level behavior. Either the parser somehow needs to fill freeName to the className or the hook calling might need adoption to work with Identifiers.

Other relevant information:
webpack version: 5.90.3
Node.js version: v20.9.0
Operating System: Windows 11 Professional
Additional tools:

@Danielku15
Copy link
Author

Danielku15 commented Feb 23, 2024

I found something suspicious which might allow an easy fix:

  1. Various top level symbols are marked by the InnerGraphPlugin:
    const fn = InnerGraph.tagTopLevelSymbol(parser, name);
  2. The InnerGraph.tagTopLevelSymbol first defines a variable in the parser with the name and then additionally tags the variable with the top level symbol.
    parser.defineVariable(name);
    const existingTag = /** @type {TopLevelSymbol} */ (
    parser.getTagData(name, topLevelSymbolTag)
    );
    if (existingTag) {
    return existingTag;
    }
    const fn = new TopLevelSymbol(name);
    parser.tagVariable(name, topLevelSymbolTag, fn);
  3. The problem:
    1. parser.defineVariable adds a variable with the current scope as value.
    2. parser.tagVariable also defines a variable but with the correct top level symbol as value. But as there is already a VariableInfo registered from defineVariable resulting in the name not to be set

I see two potential fixes in this code path:

  1. In InnerGraph.tagTopLevelSymbol we remove the call to parser.defineVariable. (or move it into the if-statement when a tag already exists).
    • Feels like the safer option as the change is quite isolated and just ensures we do not double-define the variable with missing info.
  2. In JavaScriptParser.tagVariable we fill the name we know instead of true into the VariableInfo.
    • It seems the old name was used before and things were refactored as part of ec51894 I'm not so sure about the impact of this change.

@alexander-akait
Copy link
Member

Feel free to send a PR, we have a lot of tests, so we will catch potential problems

@alexander-akait
Copy link
Member

alexander-akait commented Feb 27, 2024

Just looked at the reproducible test repo, yeah, we need to fix it, because other plugins can be broken, it can work for the development mode, but broken for the production mode, it is a critical issue... Will you look at this?

@Danielku15
Copy link
Author

Yes, I will try to open a PR soon. Should be there the next few days latest. Locally solution 1 worked fine for me. But have to check all unit tests first.

@Danielku15 Danielku15 linked a pull request Feb 27, 2024 that will close this issue
@Danielku15
Copy link
Author

Made a proposal for fixing the problem. I'm willing to contribute the change as an individual developer but I'm a bit reluctant to provide so many personal details (e.g. mailing address) in the EasyCLA to just contribute some code to this project.

@VivekShahare04
Copy link

I wanted to resolve this issue

@VivekShahare04
Copy link

see if we directly adjust the hook registration logic to work with identifiers ,
(node:8984) [DEP_WEBPACK_COMPILATION_NORMAL_MODULE_LOADER_HOOK] DeprecationWarning: Compilation.hooks.normalModuleLoader was moved to NormalModule.getCompilationHooks(compilation).loader

it will give you just deprecation warning ,so if incase we adjust it so that we're tapping into the expression hook for the "new" keyword. Then, we check if the argument to new is an identifier (assuming it's a simple class name without further qualification) and directly check if it matches "NotWorkingClass"
This approach circumvents the need for the parser to fill freeName correctly and directly handles the class name. Try this adjustment in your code to see if it resolves the issue with NotWorkingClass expressions not triggering the hook.

I am going to attach my piece of code......
Screenshot (44)

tell me if it helps you if not then we will work on this issue more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Priority - High
Development

Successfully merging a pull request may close this issue.

4 participants