X Tutup
Skip to content

feat(url_resolver): support package: urls (fixes #2991)#3215

Merged
yjbanov merged 1 commit intoangular:masterfrom
yjbanov:package-url-support
Jul 24, 2015
Merged

feat(url_resolver): support package: urls (fixes #2991)#3215
yjbanov merged 1 commit intoangular:masterfrom
yjbanov:package-url-support

Conversation

@yjbanov
Copy link
Copy Markdown
Contributor

@yjbanov yjbanov commented Jul 23, 2015

Fixes #2991

@yjbanov yjbanov self-assigned this Jul 23, 2015
@yjbanov yjbanov added this to the alpha-33 milestone Jul 23, 2015
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would be nice if we could choose the package path via config. Would that be possible here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah can you include this into the bindings. it would be nice to provide an array of conventions.

bind(URL_CONFIG).toValue([
  { package: 'node',  url:'/node_modules/' },
  { package: 'bower', url:'/bower_components/' },
  { package: 'web',   url:'/web_modules/' }
])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, please make this configurable via an OpaqueToken. Maybe use @Optional and throw if not set and packages is used, as I doubt that people really serve their node_modules folder in production...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Having never used bower (and not sure where web_modules are coming from, webpack?) I would not be able to verify that it works properly, so I'll punt on this here but I logged #3257 so we do not forget.

@yjbanov yjbanov force-pushed the package-url-support branch 4 times, most recently from 05399cd to 01a820f Compare July 23, 2015 23:53
@yjbanov yjbanov changed the title WIP feat(url_resolver): support package: urls (fixes #2991) feat(url_resolver): support package: urls (fixes #2991) Jul 24, 2015
@yjbanov yjbanov added feature Label used to distinguish feature request from other issues comp: core action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Jul 24, 2015
@yjbanov yjbanov force-pushed the package-url-support branch from 01a820f to db362fc Compare July 24, 2015 00:30
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we not have this method here? I.e. the getOuterHTML for the browser_adapter.ts does not include this logic. Instead, a user should just do what you do in the implementation: DOM.getOuterHTML(document.documentElement), which works in Dart and JS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

gone

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you make the UrlResolver configurable via a token, we don't need a custom UrlResolver for the demos at all!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

leaving UrlResolver untouched, so this still makes sense.

@tbosch
Copy link
Copy Markdown
Contributor

tbosch commented Jul 24, 2015

As talked offline: for now let’s just add the package schema for Dart and change the DemoUrlResolver to make the examples work in JS and Dart.

@tbosch tbosch added pr_state: LGTM action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 24, 2015
@yjbanov yjbanov force-pushed the package-url-support branch from db362fc to ff9bd6e Compare July 24, 2015 01:06
@yjbanov yjbanov force-pushed the package-url-support branch from ff9bd6e to 408618b Compare July 24, 2015 01:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: merge The PR is ready for merge by the caretaker cla: yes feature Label used to distinguish feature request from other issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement standard universal absolute schema for Angular 2 resource URI

7 participants

X Tutup