-
Notifications
You must be signed in to change notification settings - Fork 361
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
add support for amd options #663
base: master
Are you sure you want to change the base?
Conversation
I'm a little worried about these AMD-specific parameters. Out of curiosity - did you try using |
They are not the same. in with name=moda typeof define === 'function' && define.amd ? define(factory) :
(global = global || self, global.moda = factory()); with amd.id=modb typeof define === 'function' && define.amd ? define('modb', factory) :
(global = global || self, global.mod = factory()); with both typeof define === 'function' && define.amd ? define('modb', factory) :
(global = global || self, global.moda = factory()); |
As a thought, what about having an |
If possible, I'd love to get a better understanding of why named define is useful. I've written a few AMD loaders in the past and only ever needed named defines or custom define functions as a workaround. |
Co-authored-by: Jason Miller <[email protected]>
Co-authored-by: Jason Miller <[email protected]>
In case of |
@hanai We feel that adding this feature adds a bit more complex than we want to microbundle. For your use-case, it would make more sense to go with rollup directly instead of microbundle. We do see value in adding support for creating named amd modules. If you could refactor this PR to support the Thanks for understanding and contributing to microbundle! |
@wardpeet @developit I'm running into this issue too — i.e. the lack of support for the amd rollup options. I was thinking submitting a PR and found this one. The The amd.id is pretty important since there are certain module loaders that don't support anonymous definitions. Also, when writing a custom module loader, the id that comes to the define function is useful in countless ways — up to the lib developer. Supporting these options would be suuuuper simple — could be as simple as adding outputOptions: {
...
// if options.amdId or options.amdDefine is undefined rollup takes it as not supplied and behaves as default
amd: {
id: options.amdId,
define: options.amdDefine
},
...
}
As an example, you may have the define function under a certain namespace (e.g. myCompany.define) so amd.define = myCompany.define would give you the right output, otherwise it will always call define on the global object and fail if it isn't there. I'm happy to submit a PR once I hear back from you guys that it would be accepted. Please suggest approach too to minimize PR revision back-and-forth. |
Would be keen to see this! Running into this when packaging a library for use in Decentraland, which requires named AMD modules with the ID being the same as the NPM package name. FWIW, I'd also be keen to have the flexibility to separately define the Understand that this increases the surface area of microbundle, though I think it's a worthwhile addition that unlocks an edge case, without requiring authors to eject from microbundle and roll their own rollup config. Would you be open to this PR as-is if I resolved the merge conflicts? What do you think? Thank you! 🙌 |
I have the same issue. If the module is un-named. There will be an issue working with the require.js. My use case is we are writing 3rd party widget. Then using our script on a site is using require.js already(and their root script is un-named too) and will see this error. |
add support for amd options
https://rollupjs.org/guide/en/#outputamd