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

Fix requirejs #292

Closed
wants to merge 2 commits into from
Closed

Fix requirejs #292

wants to merge 2 commits into from

Conversation

iammerrick
Copy link
Contributor

Fix require.js to support global style configuration and multiple context baseUrl configuration.

See https://github.com/vojtajina/testacular/issues/291

@iammerrick iammerrick mentioned this pull request Jan 19, 2013
@dignifiedquire
Copy link
Member

Thanks for the effort, while I see the need for this I've got a few things two ask

  • Is requirejs.s.contexts stable and/or documented somewhere that it's save to use? I couldn't find anything on this
  • Please remove the addition of yourself to the contributors, this list gets generated automatically.
  • Please rebase onto latest master.
  • Please squash into one commit following the new commit style.
  • Please add an e2e test that verifies that this addition really does what it's supposed to do.

@vojtajina
Copy link
Contributor

Hey @iammerick - I'm not sure I understand the problem correctly.

Why can't you have a separate file (per environment), that would call require.config({...}), instead of publishing a global require object ?

That allows you to override the default baseUrl config (set by the adapter) with anything you like. However, with your fix, it will always prepend /base. Note that in some special cases, you might have other files, served from outside your project and then, these files won't be in /base, but /absolute/.....

@iammerrick
Copy link
Contributor Author

The benefit of the global require object is it allows you to store common and shared configuration across all your environments instead of just one. This includes the ability to use mainConfigFile for your build task...

@vojtajina
Copy link
Contributor

I still don't see any valid reason for this change. I believe it's better to have require.config({...}); in a separate file and have a different file per environment.

I'm happy to discuss some fix, if this is really issue for more people. However this fix relies on internals of requirejs and disable oportunity to load files from other than /base prefix.

So I'm closing it for now.

@vojtajina vojtajina closed this Feb 3, 2013
vojtajina added a commit to vojtajina/karma that referenced this pull request Feb 16, 2013
I believe in most cases people need to use different baseUrl anyway. This means
people need to understand that Testacular serves files under `/base/*` (which is
the basePath from the testacular.conf.js) and under `/absolute/*` (files from
outside of the project's basePath). But it's less magic and less confusing.

See karma-runner#291, karma-runner#292 for more info.

Closes karma-runner#291
@vojtajina
Copy link
Contributor

Hey @iammerrick - check out #291 and let me know, what you think.

Apologies, I didn't understand your problem correctly.

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

Successfully merging this pull request may close these issues.

3 participants