Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

feat: make codebase more modular so that only parts of it can be loaded#748

Merged
mhevery merged 1 commit intoangular:masterfrom
mhevery:modular
Apr 26, 2017
Merged

feat: make codebase more modular so that only parts of it can be loaded#748
mhevery merged 1 commit intoangular:masterfrom
mhevery:modular

Conversation

@mhevery
Copy link
Copy Markdown
Contributor

@mhevery mhevery commented Apr 22, 2017

No description provided.

@mhevery
Copy link
Copy Markdown
Contributor Author

mhevery commented Apr 22, 2017

@JiaLiPassion I am working on speeding up the loading of the zone as well as making the loading more modular. This is a start, would love to have your feedback.

@mhevery mhevery requested a review from JiaLiPassion April 22, 2017 04:46
@JiaLiPassion
Copy link
Copy Markdown
Collaborator

@mhevery , got it, thank you for involving me, I will look into it.

Comment thread lib/zone.ts
/** @internal */
type AmbientZoneDelegate = ZoneDelegate;

const Zone: ZoneType = (function(global: any) {
Copy link
Copy Markdown
Collaborator

@JiaLiPassion JiaLiPassion Apr 22, 2017

Choose a reason for hiding this comment

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

@mhevery , Should we separate the Zone interface and implementation into separate files?

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.

We could. What would be the advantage?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just to clarify the interface and implementation and reduce the zone.ts size, it is a little big, user may easily check the API from the interface file.

But we have the zone.js.d.ts file, so I think it is also ok in current way.

Comment thread lib/zone.ts
};
};
const patches: {[key: string]: any} = {};
const _api: _ZonePrivate = {
Copy link
Copy Markdown
Collaborator

@JiaLiPassion JiaLiPassion Apr 22, 2017

Choose a reason for hiding this comment

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

should we expose those API in two kinds

  1. util method as symbol
    other module can just use it.

  2. listeners as consoleError
    other module can add listener to it. I think maybe there are more modules want to handle unhandled microtask error for example.

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.

  1. I want to keep in hidden so that you can only get to it from the callback.

  2. We could do that, but since we only have one place which needs it so far, I would prefer to have a use case before we complicate things.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Got it. thanks.

Comment thread lib/browser/browser.ts
function findPendingTask(target: any) {
const pendingTask: Task = target[XHR_TASK];
return pendingTask;
Zone.__load_patch('timers', (global: any, Zone: ZoneType, api: _ZonePrivate) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like this one!!!, this is much more clear!!!
We should also patch other non-standard libraries in this way, such as

  • Notification
  • MediaQuery
  • 3rd party promise
    ...

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.

Yes, that is my plan, just have not gotten to it all.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Got it!!!

Comment thread lib/common/utils.ts Outdated
// so we should use original native get to retrieve the handler
let value = originalDescGet.apply(this);
let value = originalDescGet && originalDescGet.apply(this);
value = desc.set.apply(this, [value]);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just for remind, this may need to use the #747 's modification

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.

#747 is merged, so it should pick it up on rebase.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sure!

Comment thread lib/zone.ts
return _currentTask;
};

static __load_patch(name: string, fn: _PatchFn): void {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should we allow 3rd party add their own patch?
If so, we may need to consider which Zone Private API should we expose.

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.

I would prefer to keep this private to zone for now. Would love to see some use cases before we expose it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree.

Comment thread lib/zone.ts
}

function drainMicroTaskQueue() {
const showError: boolean = !(Zone as any)[__symbol__('ignoreConsoleErrorUncaughtError')];
Copy link
Copy Markdown
Collaborator

@JiaLiPassion JiaLiPassion Apr 22, 2017

Choose a reason for hiding this comment

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

Do we need to organize all symbol to some const definition block or separate file?
It is a little hard to find what are defined or used...

One more thing, should we define some ZoneConfiguration to contains all Zone settings
such as

{
    ignoreConsoleErrorUncaughtError: true
}

or

interface ZoneSettings {
    ignoreConsoleErrorUncaughtError: boolean
}

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.

Yes that sounds like a good idea. Perhaps we could do that as a second pass after this?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, sure!

Comment thread lib/zone.ts Outdated
if ((Zone as any)[__symbol__('ignoreConsoleErrorUncaughtError')]) {
return;
}
_api.consoleError(showError, error);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should we define this API as

processUnhandleMicrotaskError(...)

or something like that?

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.

good, idea changed.

@mhevery mhevery force-pushed the modular branch 3 times, most recently from f25033b to 5258a1d Compare April 26, 2017 22:15
@marinho
Copy link
Copy Markdown

marinho commented Jan 10, 2018

I just realised this my comment should be moved into an issue, so, I here it is - #991

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants