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

Generate index.js from a template #1483

Merged
merged 3 commits into from Nov 7, 2017
Merged

Generate index.js from a template #1483

merged 3 commits into from Nov 7, 2017

Conversation

aearly
Copy link
Collaborator

@aearly aearly commented Oct 15, 2017

I noticed we were leaving out some aliases in our index.js. It actually was not possible to const async = require('async'); async.find(...).

To work around this, I decided to build lib/index.js from a template, reading lib/ and a static aliases.json to fill it in. This also makes maintaining lib/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.

@aearly aearly requested review from megawac and hargasinski Oct 15, 2017
@megawac
Copy link
Collaborator

@megawac megawac commented Oct 15, 2017

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.

I'd rather we punt on this and drop the aliases in v3 (and make things like detect -> find where appropriate)

@aearly
Copy link
Collaborator Author

@aearly aearly commented Oct 16, 2017

I think aliases are fine. And in 2.0, some are actively broken, still needing fixing.

@megawac
Copy link
Collaborator

@megawac megawac commented Oct 16, 2017

I think things like detect should definitely be dropped. That alias is very rare and non standard in other similar libs/es2015

@aearly
Copy link
Collaborator Author

@aearly aearly commented Oct 16, 2017

What do you think about these changes to solve our current bug?

@aearly
Copy link
Collaborator Author

@aearly aearly commented Oct 16, 2017

I agree about detect, select, and inject, but with the others it's less clear as to which is "correct".

@hargasinski
Copy link
Collaborator

@hargasinski hargasinski commented Oct 16, 2017

For adding the missing aliases, this LGTM. I'm not a huge fan of the static aliases.json though.

I'm good with dropping less commonly used aliases in v3, but ones like forEach, and forEachOf we should definitely keep.

@@ -45,6 +47,9 @@ build-bundle: build-modules $(UMD_BUNDLE)

build-modules: $(CJS_MODULES)

$(JS_INDEX): $(INDEX_SRC)
Copy link
Collaborator

@hargasinski hargasinski Oct 16, 2017

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?

Copy link
Collaborator

@hargasinski hargasinski Oct 16, 2017

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
Copy link
Collaborator

@hargasinski hargasinski Oct 16, 2017

Choose a reason for hiding this comment

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

nit: process.nextTick

@aearly
Copy link
Collaborator Author

@aearly aearly commented Oct 17, 2017

@hargasinski Would you prefer reading the @alias foo directives in the doc blocks vs. a static aliases.json?

@aearly
Copy link
Collaborator Author

@aearly aearly commented Oct 17, 2017

Actually, I think aliases.json is best, because I can parse/stringify it in such a way that it can become a list of files to generate with Make. (I could generate it with another script, though...)

@hargasinski
Copy link
Collaborator

@hargasinski hargasinski commented Oct 18, 2017

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.

megawac
megawac approved these changes Nov 4, 2017
@aearly aearly merged commit 777ca0f into master Nov 7, 2017
3 checks passed
@aearly aearly deleted the generate-index branch Nov 7, 2017
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

3 participants