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.

Sign up for GitHub

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

Jump to bottom

Create official loader module API. #118

Closed
wants to merge 2 commits into from
Closed

Create official loader module API. #118

wants to merge 2 commits into from

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Apr 18, 2017

Provides an internal loader module with the following named exports:

  • getIds - Returns an array containing the id's of all registered
    modules.
  • has - Returns true or false if a given module exists.
  • define - Defines a new module.
  • require - Requires a module (optionally relative to the current module).
  • resolve - Returns the expanded module name given a relative path.

Closes #82.

@rwjblue
Create official loader module API.
10d2c2a 
Provides an internal `loader` module with the following named exports:

* `getIds` - Returns an array containing the `id`'s of all registered
  modules.
* `has` - Returns `true` or `false` if a given module exists.
* `define` - Defines a new module.
* `require` - Requires a module (optionally relative to the current module).
* `resolve` - Returns the expanded module name given a relative path.
@stefanpenner
Copy link
Contributor

stefanpenner commented Apr 18, 2017 •
edited
Loading

how does this differ from import require from 'require'; require and require.has? Is this just that + more API?

@rwjblue
Copy link
Member Author

rwjblue commented Apr 18, 2017

@stefanpenner - require and require.has are basically the same, the other methods do not have public API's today. Also, I think import { has, require } from 'loader' is better 😝 ...

@rwjblue
Copy link
Member Author

rwjblue commented Apr 18, 2017

IMHO, we should probably eventually deprecate import require from 'require' (unless that is a standard part of the AMD spec?), in favor of import { require } from 'loader';. Since the latter case is quite a bit easier to track down who provides it.

I'm not suggesting any deprecations just yet, just pondering....

@rwjblue
Copy link
Member Author

rwjblue commented Apr 18, 2017

Also added some documentation for the new API.

trentmwillis
trentmwillis approved these changes Apr 18, 2017
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

Overall this seems good to me, just one comment about naming (not super important).

README.md
/*
Return the list of module id's that are present in the registry
*/
getIds(): string[];
Copy link
Member

Choose a reason for hiding this comment

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

May just be me, but for some reason, I do not think id when I think of the identifier for a module. Usually path or name instead. When I first saw getIds() I had no idea what information it was actually returning.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the unique id, usually name is relative or not where as this is registry specific.

https://whatwg.github.io/loader/#loader-import if you look here name is used for APIs like import() but the registry uses keys.

Would getKeys() be ok with you?

I hate naming things.

@rwjblue rwjblue mentioned this pull request Apr 19, 2017
Closed
@rwjblue
Copy link
Member Author

rwjblue commented May 16, 2017

Where are we on this? I believe that all of the comments here have been addressed. What other blockers are there?

@rwjblue
Copy link
Member Author

rwjblue commented May 17, 2017

@stefanpenner / @krisselden - Are y'all ok with this? Any changes needed?

@stefanpenner
Copy link
Contributor

stefanpenner commented May 17, 2017 •
edited
Loading

I'm not terribly thrilled to:

  • take over the loader entry
  • duplicate/behavior API's that already exist on require

Is there a reason we can't just add more things to require?

@rwjblue
Copy link
Member Author

rwjblue commented May 17, 2017

I'm not terribly thrilled to:

  • take over the loader entry

We can handle folks overriding via define('loader', so that their loader module wins but with a deprecation.

  • duplicate/behavior API's that already exist on require

I'm happy to deprecate require here as well.

Is there a reason we can't just add more things to require?

A couple things that pop out at me:

  • using the require module for things like define or define.alias which are completely unrelated to requiring things is pretty weird
  • this packages name is loader.js, using require as our primary module API is bizarre and non-intuitive.

@stefanpenner
Copy link
Contributor

I'm happy to deprecate require here as well.

I don't think we can, as require is a legitimate import AMD import, and in AMD land is already used to signal capabilities and other things.

I still feel we should just expand the API on require and not introduce a different thing.

@Turbo87
Copy link
Member

Turbo87 commented Feb 16, 2019

closing due to inactivity

@Turbo87 Turbo87 closed this Feb 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trentmwillis trentmwillis trentmwillis approved these changes

@stefanpenner stefanpenner Awaiting requested review from stefanpenner

@krisselden krisselden Awaiting requested review from krisselden

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cleanup entries API
5 participants
@rwjblue @stefanpenner @Turbo87 @krisselden @trentmwillis