-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(ModuleImporter): implement import module compatible with bundlers #6709
Conversation
Sorry, just removed the non-working Rollup implementation that was dangling in my first commit 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be done for index.js too? Also doesn't the class need to be declared in the typings?
I think, as long as this util is only used internally (also marked as For |
Yeah I thought all classes were in the typings but they're not so scrap that
You could probably pass an object with the name of the file as a key and an array (or string?) with the class(es) that should be exported from that file and it would save us a lot of headaches, just gotta see what classes are not exported |
This needs a rebase. |
This needs a rebase |
Implement ModuleImporter, which dynamically includes modules based on the used bundler or file system. Change ActionsManager to use the new util instead of directly accessing the file system. close discordjs#6681
Fixed a flipped condition, which would include exactly the wrong modules when using the ModuleImporter with Webpack.
Add 'node:' prefix to node imports in ModuleImporter.js
Rebased and adapted imports to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FS still works fine. Webpack seems sounds, but untested by me.
Please describe the changes this PR makes and why it should be merged:
In this PR, I implemented a ModuleImporter helper class, which currently supports Webpack and File System Imports.
The Util can import all files from a given directory without listing them manually and thus circumvents missed imports.
In an earlier approach [#6102] this issue was solved by doing a file system search, which currently is the only change preventing the use of code bundlers (Webpack, Rollup, etc) in d.js@>13. Read this comment on why bundling is important for some use cases other than Web Development.
My approach will actually just work for File Sytsem import or Webpack, because I'm not very familiar with other bundlers and the implementation seemed not so straightforward. Potentially Rollup works out-of-the-box with rollup-plugin-require-context.
I tested both file system and WebPack in my projects.
I'm open for discussion and feedback.
Close #6681
Status and versioning classification: