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

refactor: change import for webpack v5 | not ready #169

Closed

Conversation

deleonio
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Nov 13, 2020

CLA assistant check
All committers have signed the CLA.

@deleonio
Copy link
Author

image

@deleonio deleonio changed the title refactor: change import for webpack v5 refactor: change import for webpack v5 | not ready Nov 13, 2020
@jods4
Copy link
Contributor

jods4 commented Nov 13, 2020

@martinoppitz Thank you for having a go at this, much appreciated!

I see this is not ready but I'll make two initial comments:

  1. Please don't commit dist. It is built and committed by the release process.

  2. It's great that you tried to maintain compat with v4 as well. If that becomes too hairy, just drop it. We'll make a plugin v5 release compatible with webpack 5, and webpack 4 users can stay on the plugin v4.

@deleonio deleonio force-pushed the fix/webpack-v5-breaking-cange branch from 4c14f08 to ee8e8d3 Compare November 14, 2020 02:08
@deleonio
Copy link
Author

Hello @jods4,

in my first commit I focused on v5 upgrade. Now I reset my v4 changes and pushed it again.

Now we must look at the iterator problem.

@deleonio
Copy link
Author

Hi @jods4 and @EisenbergEffect,

in the plugin an internal webpack.d.ts file is used instead of the one from webpack itself.

there seem to be some changes here that i can't fix on my own.

@jods4
Copy link
Contributor

jods4 commented Nov 16, 2020

@martinoppitz I wrote that .d.ts file to augment the public one.
I haven't checked recently but at the time this plugin was first written, only common "webpack-user" APIs were documented in its typescript definitions.
Practically none of the "internal" API geared at "plugin-writters" were typed.
So I augmented the public definitions with the stuff I needed to use to create this plugin.

These definitions are almost guaranteed to not be in sync with the Webpack 5.
You can remove anything that's now in the official webpack.d.ts, doesn't exist anymore, or isn't used by the plugin -- and figure out the correct type defs for whatever you use that's not in the official one.

@deleonio
Copy link
Author

deleonio commented Dec 3, 2020

@deleonio
Copy link
Author

Hello @jods4.

unfortunately i don't have time to get the webpack plugin running.

We must upgrade webpack to v5. Because a lot of plugins only support webpack v5 in the future.

Do you already have a plan?

@ckotzbauer
Copy link
Sponsor

I started investigating here and used the changes from this PR as base. I changed the code of PreserveExportsPlugin.ts and PreserveModuleNamePlugin.ts to fix the "reasons not iterable" problem (I don't know if I really fixed it, but the error is gone). But now I get the next error-message from webpack:

Module not found: Error: Can't resolve './src' in '/mnt/dev/aurelia-client'
resolve './src' in '/mnt/dev/aurelia-client'
  using description file: /mnt/dev/aurelia-client/package.json (relative path: .)
    Field 'browser' doesn't contain a valid alias configuration
    using description file: /mnt/dev/aurelia-client/package.json (relative path: ./src)
      no extension
        Field 'browser' doesn't contain a valid alias configuration
        /mnt/dev/aurelia-client/src doesn't exist
      .ts
        Field 'browser' doesn't contain a valid alias configuration
        /mnt/dev/aurelia-client/src.ts doesn't exist
      .js
        Field 'browser' doesn't contain a valid alias configuration
        /mnt/dev/aurelia-client/src.js doesn't exist
      as directory
        /mnt/dev/aurelia-client/src doesn't exist

webpack 5.21.1 compiled with 2 errors in 1030 ms

There is no src folder, as the aurelia app is located in a app folder. But I don't see any parameter of this plugin or webpack itself to change this. Does anybody know? 🤔

@jods4
Copy link
Contributor

jods4 commented Feb 8, 2021

@ckotzbauer Hard to say from this error stack but have you tried changing the default options?
viewFor assumes a src folder:
https://github.com/aurelia/webpack-plugin/wiki/AureliaPlugin-options#viewfor-and-viewextensions

@ckotzbauer
Copy link
Sponsor

I explicitly set viewsFor to app/**/*.{ts,js}. There are no other options of this plugin with a src default value:

this.options = Object.assign({
      includeAll: <false>false,            
      aureliaConfig: ["standard", "developmentLogging"],
      dist: "native-modules",
      features: { },
      moduleMethods: [],
      noHtmlLoader: false,
      // Undocumented safety switch
      noInlineView: false,
      noModulePathResolve: false,
      noWebpackLoader: false,
      // Ideally we would like _not_ to process conventions in node_modules,
      // because they should be using @useView and not rely in convention in 
      // the first place. Unfortunately at this point many libs do use conventions
      // so it's just more helpful for users to process them.
      // As unlikely as it may seem, a common offender here is tslib, which has
      // matching (yet unrelated) html files in its distribution. So I am making 
      // a quick exception for that.
      viewsFor: "**/!(tslib)*.{ts,js}",
      viewsExtensions: ".html",
    },
    options);

@gowrav-scienaptic
Copy link

@ckotzbauer, Can you please post the changes you have done to PreserveExportsPlugin.ts and PreserveModuleNamePlugin.ts

@ckotzbauer
Copy link
Sponsor

Of course @gowrav-scienaptic 👍

PreserveExportsPlugin.ts

        for (let module of modules) {
+          for (let dep of module.dependencies) {
-          for (let reason of module.reasons) {
-            let dep = reason.dependency;
            let imports = dep[dependencyImports];
            if (!imports) continue;            
            let set = getModuleExports(module);
            for (let e of imports)
              set.add(e);
          }
        }

PreserveModuleNamePlugin.ts

function getPreservedModules(modules: Webpack.Module[]) {      
  return new Set(
    modules.filter(m => {
      // Some modules might have [preserveModuleName] already set, see ConventionDependenciesPlugin.
      let value = m[preserveModuleName];      
+      for (let d of m.dependencies) {
+        if (!d || !d[preserveModuleName]) continue;
-      for (let r of m.reasons) {
-        if (!r.dependency || !r.dependency[preserveModuleName]) continue;
        value = true;
+        let req = removeLoaders((d as ModuleDependency).request);
-        let req = removeLoaders((r.dependency as ModuleDependency).request);
        // We try to find an absolute string and set that as the module [preserveModuleName], as it's the best id.
        if (req && !req.startsWith(".")) {
          m[preserveModuleName] = req;
          return true;
        }
      }
      return !!value;
    })
  );
}

@graycrow
Copy link
Contributor

... But now I get the next error-message from webpack:

Module not found: Error: Can't resolve './src' in '/mnt/dev/aurelia-client'
resolve './src' in '/mnt/dev/aurelia-client'
...

There is no src folder, as the aurelia app is located in a app folder. But I don't see any parameter of this plugin or webpack itself to change this. Does anybody know? thinking

I have exactly the same error, but I have the default ./src directory.

@gowrav-scienaptic
Copy link

module.reasons can't be replaced with module.dependencies as shown above. Tried it on Aurelia skeleton app and it doesn't work. Unable to find the equivalent of module.reasons in webpack 5.

@bigopon
Copy link
Member

bigopon commented Apr 4, 2021

Should reasons of a module be understood as issuers or dependencies?

@Devvox93
Copy link

Devvox93 commented Apr 9, 2021

I have done some searching and (possibly) found a solution/workaround for the removal of module.reasons in this comment.

I was checking module.reasons but it was removed in webpack 5. My workaround was calling compilation.getStats() which still contains the reasons...

Update 1: I have fooled around a bit with the source, but I'm not educated enough in how Webpack works exactly to work this out.
Perhaps @ScriptedAlchemy can shed some light on how to get the reasons (or similar information) in Webpack 5.
The release notes simply state "Module.reasons removed", without any info on migration or alternatives.

Update 2: In #166 bigopon is working on supporting webpack 5 the proper way. I suspect this PR will become unnecessary.

@bigopon
Copy link
Member

bigopon commented Apr 30, 2021

Here is my current draft for the needed changes: #170. The work needed for all the plugins is listed in 3 simple parts for now: typings & code & comments. Overtime, more details will be filled in. Will update my progress there as I continue the refactoring.

@deleonio
Copy link
Author

Its done ... #170

@deleonio deleonio closed this Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants