-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Generate index.js from a template #1483
Conversation
I'd rather we punt on this and drop the aliases in v3 (and make things like |
I think aliases are fine. And in 2.0, some are actively broken, still needing fixing. |
I think things like |
What do you think about these changes to solve our current bug? |
I agree about |
For adding the missing aliases, this LGTM. I'm not a huge fan of the static I'm good with dropping less commonly used aliases in v3, but ones like |
@@ -45,6 +47,9 @@ build-bundle: build-modules $(UMD_BUNDLE) | |||
|
|||
build-modules: $(CJS_MODULES) | |||
|
|||
$(JS_INDEX): $(INDEX_SRC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be added as a dependency of build-dist
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it already is, sorry.
@@ -4,7 +4,7 @@ import { hasNextTick, hasSetImmediate, fallback, wrap } from './internal/setImm | |||
|
|||
/** | |||
* Calls `callback` on a later loop around the event loop. In Node.js this just | |||
* calls `setImmediate`. In the browser it will use `setImmediate` if | |||
* calls `process.nextTicl`. In the browser it will use `setImmediate` if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: process.nextTick
@hargasinski Would you prefer reading the |
Actually, I think |
That's a good point. Plus, after thinking about it a little more, the file will rarely change, so it might not even be worth the effort to write another script to generate it. I'm good with these changes. |
I noticed we were leaving out some aliases in our
index.js
. It actually was not possible toconst async = require('async'); async.find(...)
.To work around this, I decided to build
lib/index.js
from a template, readinglib/
and a staticaliases.json
to fill it in. This also makes maintaininglib/index.js
easier. I am also considering not checking it in.We do not have to have an
aliases.json
, we could also parse source files for@alias foo
directives in the JSDoc blocks, but want some feedback on that strategy.We also are missing individual module files for every alias. You cannot
require('async/find')
. I'm going to add this, once I figure out how to specify it with Make.