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

UMD tests #13843

Closed
wants to merge 9 commits into from
Closed

UMD tests #13843

wants to merge 9 commits into from

Conversation

hnrch02
Copy link
Collaborator

@hnrch02 hnrch02 commented Jun 17, 2014

As suggested in #13811, here are tests/examples of using Bootstrap with RequireJS and Browserify. Both working with the concatenated file and using individual plugins is being demonstrated.

This also includes a fix for #13812, which removes the module definitions from the individual plugins and wraps them all in one big factory function. It's pretty hacky so I'd like to get feedback on it.

/cc @fat @cvrebert @XhmikosR

@cvrebert cvrebert added the js label Jun 17, 2014
@cvrebert cvrebert added this to the v3.2.0 milestone Jun 17, 2014
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 17, 2014

Added Browserify tests. Not sure whether or not to include the bundle.js that browserify generates, I included it for now.

@XhmikosR
Copy link
Member

I can't comment on all the changes at the moment but see #13842 (comment)

@cvrebert
Copy link
Collaborator

@hnrch02 Please rebase; this currently has merge conflicts.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 18, 2014

Should be rebased now.

I want to know if the level of hackyness is acceptable and if not how we could improve it.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 18, 2014

Rebased again, now that #13842 has been merged.

var footer = ' })\n\n' +
'}();\n'

return src.replace(umd, '').replace(footer, '}();\n')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use RegExp.quote here?
Yeah, hacky indeed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we need that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process runs before each file is added to the concatenated output, so unless the UMD stuff is included twice in a source file, I don't see a problem.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 18, 2014

I'll squash the commits once this is ready to merge.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 19, 2014

Also added the fix to concatenated files built using the customizer.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 25, 2014

So how are we gonna go about this? Will we revert the revert after 3.2 is released and work from there or are we moving the UMD stuff to a separate branch?

@mdo
Copy link
Member

mdo commented Aug 2, 2014

Punting to v4 checklist.

@mdo mdo closed this Aug 2, 2014
@mdo mdo removed this from the v4.0.0 milestone Aug 2, 2014
@mdo mdo mentioned this pull request Aug 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants