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

Exclude external resources from r.js optimization #208

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

Conversation

dchekanov
Copy link

Fixes #207.

@dingus9
Copy link

dingus9 commented May 17, 2016

This fix works for me please prioritize a merge

@alundiak
Copy link
Collaborator

alundiak commented Feb 18, 2017

@dchekanov I reviewed this PR a bit, and I will do more deep research how it might affect end customers. Meanwhile, if u still available for this module, could u please think on test case to write in test.js (or let me know what exactly test result u relied on).

If no reply during the week, I will recreate PR from my own with suggested code changes.

cc/ @dingus9 @linjinying @amenadiel

@dchekanov
Copy link
Author

@alundiak this plugin currently doesn't have any tests for the building part, so I'm not sure how the suggested change should be tested.

There are 2 things that this PR does to support loading modules via the paths directive:

  1. When loading a module, it checks for the final module location ('https://domain.com/file.js') and not for its name ('moduleName') to be an absolute URL.
  2. When writing module contents, it checks if the module has been loaded and not if its name is an absolute URL.

@alundiak
Copy link
Collaborator

alundiak commented Feb 23, 2017

@dchekanov

  1. Test case 1: run command r.js -o example/build.js
  2. Test case 2: edit example/build.js => optimizeCss => "standard" and run above command again.
  3. Would also great to know how buildCSS and separateCSS change affects build.

First 2 will show us at least, if all files (css.js, css-builder.js, normalize.js) working file.

And I will think about this steps later and provide pr with test for such test cases in test.js.

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