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

Import trace for requested module #1

Open
ahmedrowaihi opened this issue Jun 27, 2024 · 29 comments
Open

Import trace for requested module #1

ahmedrowaihi opened this issue Jun 27, 2024 · 29 comments

Comments

@ahmedrowaihi
Copy link

I keep getting this error in console
I am in a monorepo using PNPM & Turbo

 ⚠ ../../node_modules/.pnpm/@[email protected]/node_modules/@poppinss/utils/build/index.js
Critical dependency: the request of a dependency is an expression

Import trace for requested module:
../../node_modules/.pnpm/@[email protected]/node_modules/@poppinss/utils/build/index.js
../../node_modules/.pnpm/[email protected]_@[email protected]_@[email protected]/node_modules/flydrive/build/chunk-XTE5URPJ.js
../../node_modules/.pnpm/[email protected]_@[email protected]_@[email protected]/node_modules/flydrive/build/drivers/s3/main.js
@RomainLanz
Copy link
Member

Hey @ahmedrowaihi! 👋🏻

We need more information to help you.
The best would be to have a repository with the minimum amount of code to reproduce your issue.

@ahmedrowaihi
Copy link
Author

ahmedrowaihi commented Jun 28, 2024

Hey @RomainLanz
made one for you flydrive-trace-next-app

git clone https://github.com/ahmedrowaihi/flydrive-trace-next-app.git
pnpm install
pnpm dev

reproduce:

@RomainLanz
Copy link
Member

I had no issue locally but I have:

  • Remove the custom endpoint (since I don't have any)
  • Added the region to the .env file
  • Remove the URL generation, since it is not needed too

@ahmedrowaihi
Copy link
Author

ahmedrowaihi commented Jun 28, 2024

the S3 driver provided is just an example, you can ignore it or change it work with your s3,

the issue in the log appears for all of the drivers due to some @poppinss/utils imports

Object is getting uploaded successfully,but the logs shows this warning all the time..

Have you tried using pnpm ?
Try clean node_modules, install with pnpm, and check the reproduce steps above

@ahmedrowaihi
Copy link
Author

Logs shows in next console not browser tho

@RomainLanz
Copy link
Member

RomainLanz commented Jun 28, 2024

Alright, gotcha.

The code works fine; the issue is the log sent to the console.
I am not sure where that comes from. It is probably Turbo/Webpack.

@ahmedrowaihi
Copy link
Author

it's probably webpack, or the dist of flydrive something is conflicting

The repo I gave you above doesn't use turbo, it shows same error
also running with npm, showed the same..

something needs to be refactored, I will try to get back next week and refactor this after I get my midterm exams done

@RomainLanz
Copy link
Member

I will also check on my side to see if anything come to my mind.

Good luck with your exams! 🙌🏻

@andresgutgon
Copy link

Just starting trying flydrive this weekend. Same error in NextJS + PNPM + Turborepo

@andresgutgon
Copy link

Could be this https://github.com/poppinss/utils/blob/develop/src/import_default.ts#L19 ?

I'm reading that Webpack can't require dynamically https://stackoverflow.com/a/59235546

I've no idea tbh. Please don't get confused for my guess

@ahmedrowaihi
Copy link
Author

Could be this https://github.com/poppinss/utils/blob/develop/src/import_default.ts#L19 ?

I'm reading that Webpack can't require dynamically https://stackoverflow.com/a/59235546

I've no idea tbh. Please don't get confused for my guess

hmm, I will try to eject only needed utilities for flydrive to work without using Poppins and test it again

@RomainLanz
Copy link
Member

It is a known issue of Webpack. @Julien-R44 already made some investigation in Bentocache for the same issue.

@ahmedrowaihi
Copy link
Author

ahmedrowaihi commented Jul 1, 2024

@andresgutgon you are right, the issue was due to the dynamic require

you can use this patch i made to get rid of the issue currently, the removed methods doesn't affect flydrive, it's not used on it
these methods are removed in the patch..

  • fsImportAll
  • importDefault

patch file: @poppinss+utils+6.7.3.patch
you can use it with patch-package

diff --git a/node_modules/@poppinss/utils/build/index.js b/node_modules/@poppinss/utils/build/index.js
index 84ca468..10ba893 100644
--- a/node_modules/@poppinss/utils/build/index.js
+++ b/node_modules/@poppinss/utils/build/index.js
@@ -111,19 +111,6 @@ var RuntimeException = class extends Exception {
   static status = 500;
 };
 
-// src/import_default.ts
-async function importDefault(importFn, filePath) {
-  const moduleExports = await importFn();
-  if (!("default" in moduleExports)) {
-    const errorMessage = filePath ? `Missing "export default" in module "${filePath}"` : `Missing "export default" from lazy import "${importFn}"`;
-    throw new RuntimeException(errorMessage, {
-      cause: {
-        source: importFn
-      }
-    });
-  }
-  return moduleExports.default;
-}
 
 // src/define_static_property.ts
 import lodash from "@poppinss/utils/lodash";
@@ -242,30 +229,6 @@ function isScriptFile(filePath) {
   return false;
 }
 
-// src/fs_import_all.ts
-async function importFile(basePath, fileURL, values, options) {
-  const filePath = fileURLToPath2(fileURL);
-  const fileExtension = extname2(filePath);
-  const collectionKey = relative(basePath, filePath).replace(new RegExp(`${fileExtension}$`), "").split(sep);
-  const exportedValue = fileExtension === ".json" ? await import(fileURL, { assert: { type: "json" } }) : await import(fileURL);
-  lodash2.set(
-    values,
-    options.transformKeys ? options.transformKeys(collectionKey) : collectionKey,
-    exportedValue.default ? exportedValue.default : { ...exportedValue }
-  );
-}
-async function fsImportAll(location, options) {
-  options = options || {};
-  const collection = {};
-  const normalizedLocation = typeof location === "string" ? location : fileURLToPath2(location);
-  const files = await fsReadAll(normalizedLocation, {
-    filter: isScriptFile,
-    ...options,
-    pathType: "url"
-  });
-  await Promise.all(files.map((file) => importFile(normalizedLocation, file, collection, options)));
-  return collection;
-}
 
 // src/message_builder.ts
 var MessageBuilder = class {
@@ -406,11 +369,9 @@ export {
   createError,
   defineStaticProperty,
   flatten,
-  fsImportAll,
   fsReadAll,
   getDirname,
   getFilename,
-  importDefault,
   isScriptFile,
   joinToURL,
   naturalSort,

@andresgutgon
Copy link

andresgutgon commented Jul 1, 2024

Uhmm, would be the long term solution here?

I understand poppings do that to have sub-set of lodash functionality no?

@ahmedrowaihi
Copy link
Author

@poppings/utils is a utility used for all adonisjs libs/frameworks

in case of flydrive, it uses few utils for runtime exception and string/byte matching etc...

the thing is that @poppings/utils exports most functions at the index.ts

using my patch above won't hurt if you use any adonisjs lib like: flydrive with nextjs
unless the lib needs fsImportAll or importDefault functions, which won't work with nextjs

best solution is that we may reallocate the export of fsImportAll and importDefault to their own path, ( not in index )
so only libs that need them, can import them through @poppings/utils/import-helpers

@andresgutgon
Copy link

best solution is that we may reallocate the export of fsImportAll and importDefault to their own path, ( not in index )
so only libs that need them, can import them through @poppings/utils/import-helpers

Yes, this would be ideal

@ahmedrowaihi
Copy link
Author

ahmedrowaihi commented Jul 1, 2024

the error occurs every time flydrive-js uses an import from
@poppings/utils <- see this search result

which hits the @poppings/utils/index.ts and causes the error

@ahmedrowaihi
Copy link
Author

best solution is that we may reallocate the export of fsImportAll and importDefault to their own path, ( not in index )
so only libs that need them, can import them through @poppings/utils/import-helpers

Yes, this would be ideal

it might be good, unless other adonis libs expects those helpers from index.ts, then all those lib needs to be updated

maybe we can extend exports from package.json instead :'D

@andresgutgon
Copy link

maybe we can extend exports from package.json instead :'D

How would that work? How updating other AdonisJS packages being updated can be avoided?

@ahmedrowaihi
Copy link
Author

ahmedrowaihi commented Jul 1, 2024

applying these changes to @poppinss/utils

diff --git a/package.json b/package.json
index 446b6f7..2fd4f76 100644
--- a/package.json
+++ b/package.json
@@ -22,7 +22,10 @@
     "./string": "./build/src/string/main.js",
     "./string_builder": "./build/src/string_builder.js",
     "./json": "./build/src/json/main.js",
-    "./types": "./build/src/types.js"
+    "./types": "./build/src/types.js",
+    "./exceptions": "./build/exceptions/src/exceptions/index.js",
+    "./exception": "./build/src/exception.js",
+    "./slash": "./build/src/slash.js"
   },
   "engines": {
     "node": ">=18.16.0"
@@ -35,7 +38,7 @@
     "clean": "del-cli build",
     "typecheck": "tsc --noEmit",
     "precompile": "npm run lint && npm run clean",
-    "compile": "tsup-node && tsc --emitDeclarationOnly --declaration",
+    "compile": "tsup-node && tsc --declaration",
     "build": "npm run compile && npm run build:lodash",
     "release": "np",
     "version": "npm run build",
diff --git a/src/exceptions/index.ts b/src/exceptions/index.ts
new file mode 100644
index 0000000..138f86b
--- /dev/null
+++ b/src/exceptions/index.ts
@@ -0,0 +1,2 @@
+export * from './invalid_arguments_exception.js'
+export * from './runtime_exception.js'

will allow us to modify flydrive only without affecting other libs

for example this could be changed to:

-import { slash } from '@poppinss/utils'
+import { slash } from '@poppinss/utils/slash'

@andresgutgon
Copy link

andresgutgon commented Jul 1, 2024

But consumers of this package still needs to be updated no?

As a nextjs user I think is worthy that the package works without errors out of the box. But let's see what others say

@ahmedrowaihi
Copy link
Author

no, they don't have to,
because this added more ways to imports, it doesn't change existing imports
by adding exports to package.json

both imports are valid

import { slash } from '@poppinss/utils'
import { slash } from '@poppinss/utils/slash'

and using the second will avoid the issue for nextjs, without breaking the lib for other libs

@andresgutgon
Copy link

Ahh I got it 👌 Great. Can you do the PR?

@ahmedrowaihi
Copy link
Author

Ahh I got it 👌 Great. Can you do the PR?

Sure, only if @RomainLanz agrees on the solution suggested

@thetutlage
Copy link
Collaborator

Can I understand the root of the problem? Why does TurboPack complains about a function that performs a dynamic import or a filesystem read operation?

Sorry, I am not aware of Turbopack or internal workings of Next.js, so I am trying to understand what is the root of the warning.

@ahmedrowaihi
Copy link
Author

Can I understand the root of the problem? Why does TurboPack complains about a function that performs a dynamic import or a filesystem read operation?

Sorry, I am not aware of Turbopack or internal workings of Next.js, so I am trying to understand what is the root of the warning.

Turbopack has no problems with dynamic imports..
Webpack does, because it do static analysis for every import statement at bundle time..

So the code on the two methods above is not supported in webpack

@thetutlage
Copy link
Collaborator

Okay. In that case, I am not sure if FlyDrive is meant to be used with WebPack or any bundler for that matter.

I don't want to discourage the usage of FlyDrive, but if we end up in a cycle where we have to optimize/re-structure code to make FlyDrive work with a bundler, then I might not be eager to do so. Simply, because I want to keep my projects simple and easy to maintain for the longer run.

Now, regarding this simple change of @poppinss/utils, I will go ahead and provide a fix for it and see if everything goes smoothly beyond that. 👍

@ahmedrowaihi
Copy link
Author

Okay. In that case, I am not sure if FlyDrive is meant to be used with WebPack or any bundler for that matter.

I don't want to discourage the usage of FlyDrive, but if we end up in a cycle where we have to optimize/re-structure code to make FlyDrive work with a bundler, then I might not be eager to do so. Simply, because I want to keep my projects simple and easy to maintain for the longer run.

Now, regarding this simple change of @poppinss/utils, I will go ahead and provide a fix for it and see if everything goes smoothly beyond that. 👍

Let me know if you need help, I already suggested some fixes above

@thetutlage
Copy link
Collaborator

Yeah. I think we will go with the submodules approach (as you suggested) in @poppinss/utils.

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

No branches or pull requests

4 participants