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

add support for amd options #663

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

hanai
Copy link

@hanai hanai commented Jun 18, 2020

add support for amd options

https://rollupjs.org/guide/en/#outputamd

@developit
Copy link
Owner

I'm a little worried about these AMD-specific parameters.

Out of curiosity - did you try using --name? It should cause AMD output to use named define instead of anonymous. The amd.define option seems okay, though I'd be tempted to omit it from the documentation and just support it as a hidden feature.

@hanai
Copy link
Author

hanai commented Jun 18, 2020

They are not the same.

in package.json name=mod

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());

@developit
Copy link
Owner

As a thought, what about having an --amdNamed boolean option that causes amd.id to be set to --name?

README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@developit
Copy link
Owner

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.

hanai and others added 2 commits June 20, 2020 12:21
Co-authored-by: Jason Miller <developit@users.noreply.github.com>
Co-authored-by: Jason Miller <developit@users.noreply.github.com>
@hanai
Copy link
Author

hanai commented Jun 20, 2020

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.

In case of define is not named as define in global, such as fmd.define.

@wardpeet
Copy link
Collaborator

wardpeet commented Jul 5, 2020

@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 amdNamed boolean, that would be great.

Thanks for understanding and contributing to microbundle!

@rart
Copy link

rart commented Nov 11, 2020

@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 amdNamed option only half-solves amd.id. Rollup has those props separate, one is the name/namespace for the module and the other — the amd.id — is used on the define call to define the module with an id.

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
  },
  ...
}

@developit: 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.

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.

@developit developit added the increased scope Increases project scope, or is out of scope. label Dec 18, 2020
@bensalilijames
Copy link

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 amd.id and amd.define options as in the PR currently, instead of a --amdNamed boolean. In my case, I want to have a UMD build with the browser global having a different name from the AMD ID. If they're separate options in the CLI, I then have the power to do so!

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! 🙌

@othree
Copy link

othree commented Jul 11, 2022

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.

Reference: https://requirejs.org/docs/errors.html#mismatch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
increased scope Increases project scope, or is out of scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants