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

Support resolve hook #146

Closed
wants to merge 6 commits into from
Closed

Conversation

zhoukekestar
Copy link
Contributor

Support resolve hook. For #143

@guybedford
Copy link
Owner

Thanks for working on this. Is there a specific reason you're looking to implement an async hook or would a sync one also work? That could also simplify the implementation in folding it into the current resolver function.

@zhoukekestar
Copy link
Contributor Author

My first version is just a sync function too. But if we want some dynamic components, maybe we need to fetch its dependencies asynchronously & lazily.

For example: we want to render a WebComponent but have no dependencies (and import map) when the page loaded. Trigger ( iframe page or something ) and render this WebComponent (Dialog) lazily when we need it may be our expectation.

Maybe it's not the best practice. But someone may need this feature in the future. 😀

@guybedford
Copy link
Owner

Yeah I'm open to either. But either way I would still rather fold the resolver hook into a single call in a single resolve function.

@zhoukekestar
Copy link
Contributor Author

Then how about the API parameter defaultResolver in esmsInitOptions.resolve(id, parentUrl, defaultResolver);?
or Just scope the defaultResolver inside the resolveHook like:

async function resolveHook(id, parentUrl) {
  function defaultResolver(id, parentUrl) {}

  if ...
}

src/es-module-shims.js Outdated Show resolved Hide resolved
src/es-module-shims.js Outdated Show resolved Hide resolved
src/es-module-shims.js Show resolved Hide resolved
@zhoukekestar
Copy link
Contributor Author

Just updated the PR, simplify the resolve hook.

Copy link
Owner

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this. Still not quite there though, see the feedback further.

src/es-module-shims.js Outdated Show resolved Hide resolved
src/es-module-shims.js Outdated Show resolved Hide resolved
src/es-module-shims.js Outdated Show resolved Hide resolved
@guybedford
Copy link
Owner

The approach looks good, thanks for the work on this. See the last point about the nature of the defaultResolve function passed.

Copy link
Owner

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks great, think we've cracked it.

Re the sync / async thing this also relates to what browsers will expose via import.meta.resolve. See eg whatwg/html#3871 for discussion. Chrome currently wants to implement a sync resolver I believe. If we permit an async resolver we may be constrained in future if import.meta.resolve turns out to go sync. But we can just make a breaking change in this project too I suppose.

src/es-module-shims.js Outdated Show resolved Hide resolved
@guybedford
Copy link
Owner

The last thing before landing this would be to include the README / documentation change.

throw new Error(msg);
};

const suites = ['resolve-hook'];
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than having a separate test-resolve-hook, can we just add this to the main suite in test/test.html? That will ensure it's captured by CI.

@guybedford
Copy link
Owner

Landed in 1301af1, test refactoring and Firefox 60 fixes in c6ee672.

@guybedford guybedford closed this Aug 16, 2021
@zhoukekestar
Copy link
Contributor Author

Thanks, 🎉

@guybedford guybedford mentioned this pull request Aug 24, 2021
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.

None yet

2 participants