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

feat(ModuleImporter): implement import module compatible with bundlers #6709

Merged
merged 3 commits into from Oct 8, 2021

Conversation

nrauschcom
Copy link
Contributor

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:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

@nrauschcom
Copy link
Contributor Author

Sorry, just removed the non-working Rollup implementation that was dangling in my first commit 🙈

Copy link
Contributor

@ImRodry ImRodry left a 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?

@iCrawl iCrawl added this to the Version 13.2 milestone Sep 28, 2021
@nrauschcom
Copy link
Contributor Author

nrauschcom commented Sep 28, 2021

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 @private), adding typings serves no use.

For index.js, I think, there is no better way than listing every single export, because the decision which files are in-/excluded is not that trivial and some files only export certain submodules, e.g. Activity: require('./structures/Presence').Activity

@ImRodry
Copy link
Contributor

ImRodry commented Sep 28, 2021

I think, as long as this util is only used internally (also marked as @private), adding typings serves no use.

Yeah I thought all classes were in the typings but they're not so scrap that

For index.js, I think, there is no better way than listing every single export, because the decision which files are in-/excluded is not that trivial and some files only export certain submodules, e.g. Activity: require('./structures/Presence').Activity

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

@iCrawl
Copy link
Member

iCrawl commented Oct 2, 2021

This needs a rebase.

@vladfrangu
Copy link
Member

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
@nrauschcom
Copy link
Contributor Author

Rebased and adapted imports to node:fs and node:path.

@iCrawl iCrawl modified the milestones: Version 13.2, Version 13.3 Oct 4, 2021
Copy link
Member

@SpaceEEC SpaceEEC left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Resolving src/client/actions via filesystems breaks bundlers
7 participants