`index` event #89

piranna · 2015-12-10T10:11:14Z

Allow to render a custom index page from a function.

piranna · 2016-05-02T00:26:37Z

Any update on this?

dougwilson · 2016-05-02T00:42:37Z

index.js

-  if (listenerCount(this, 'directory') !== 0) {
-    this.emit('directory')
+  if (listenerCount(this, 'directory')) {
+    this.emit('directory', path)


Adding a new argument to the directory event does not seem related to adding an index event. Can you please restructure this pull request to only do what was described in the pull request summary and ping me when you have updated it?

piranna · 2016-08-13T12:13:12Z

I've just rebased my changes over master and tests are passing, would you be able to merge it?

dougwilson · 2016-08-13T15:18:27Z

The comment I made on your code hasn't been addressed yet. Please address it and I'll take another look :)!

…ribute

piranna · 2016-08-13T16:03:54Z

The comment I made on your code hasn't been addressed yet. Please address it and I'll take another look :)!

Sorry, I missed the comment :-P I've just fixed it and move the path argument change on a different pull-request :-)

piranna · 2016-08-14T11:46:36Z

Code rebased and tests passing. Can we focus on that over the other pull-requests? This is the most important for NodeOS at this moment...

dougwilson · 2016-08-14T14:17:30Z

Sure! Can you use typeof instead of instanceof for function checks? Why do we need both an option and an event? Is it not possible to just have one or the other?

piranna · 2016-08-14T17:29:46Z

At first I added an event like 'directory' and so, but later found it was easier to use an optional callback. In fact, if I would redesign the API, I would remove all the events except error and just use optional callbacks,
it doesn't makes sense to be able to register several functions that can return a value for a single input (the path)...

piranna · 2016-08-14T17:48:31Z

I have just changed instanceof for typeof.

dougwilson · 2016-08-14T17:53:02Z

Cool! Why do we need both an option and an event? Is it not possible to just have one or the other? If we can use events over function options that would better fit in the current API. I know the API may not be what everyone is used to, but it is what I have inherited, and it doesn't sound like you want to wait for a discussion on changing the way the API works to land this feature :) At the very least, you need to move the test you made for the index event out of the section for the index option and add documentation for that to the README as well.

dougwilson · 2016-08-14T17:54:23Z

index.js

  }

-  this._extensions = opts.extensions !== undefined
-    ? normalizeList(opts.extensions, 'extensions option')


Is this a necessary change for this PR?

normalizeList() now return an empty list when the input is undefined, so the check here is bloated.

I don't see any changes to normalizeList in your PR. Perhaps I should be clearer: don't include changes in your PR that are not accomplishing your goal. These types of changes need to be different PRs. Please revert this change.

dougwilson · 2016-08-14T17:58:31Z

README.md

-set `false` or to supply a new index pass a string or an array
-in preferred order.
+By default *send* supports "index.html" files. To disable this, set index option
+to `false`, or you can be able to supply a new index passing a string with the


"you can be able to" doesn't sound right. I think probably just remove those words?

And passing would become just pass

That's formal english... As you want.

That's formal english... As you want.

I'm sorry, I can't seem to figure out what your response means...

dougwilson · 2016-08-14T18:05:23Z

And I was jus able to sit down a s review your pull request, woot! We can get this moving along for sure :) I just had a few questions and comments; some in this conversation itself and some inline in the code.

piranna · 2016-08-14T19:07:34Z

Cool! Why do we need both an option and an event? Is it not possible to just have one or the other?

Yes, we could use just one of them, and in fact it would be better to use optional callbacks only, I just added the event to be on pair with the API, but I would remove them.

If we can use events over function options that would better fit in the current API. I know the API may not be what everyone is used to, but it is what I have inherited, and it doesn't sound like you want to wait for a discussion on changing the way the API works to land this feature :)

I'm open for a discussion about changing how the API works, it's just that this seemed to be ready to merge and wanted to use upstream code ASAP. I think events-based API should be deprecated and use optional callbacks instead, it's the correct behaviour and what everybody expects:

just one and only one listener: callback
zero or more listeners
- just one and only one call: promise
- zero or more calls: event

This also makes the API more explicit about what can and what can't do? What do you think?

At the very least, you need to move the test you made for the index event out of the section for the index option and add documentation for that to the README as well.

Where should I put the test for the index event in case we still use it?

dougwilson · 2016-08-14T19:20:35Z

We can discuss a new API style for this module, absolutely, but no new API style will be merged for months to come, because this module is locked to the release schedule of Express.js itself. Even this new feature will not get released until a minor release of Express.Js is scheduled. I wish we had all this conversation 3 months ago,when I started commenting on the changes, as it would have ended up with the 4.14 Express.js release.

I don't have time this weekend to discuss and think about an entire change to the API of this module, only have been able to sneak in reviewing these changes as a simple feature addition, as it sounded like you were interested in landing. I'll try to circle back to this PR another day when I have some time to discuss a new API :)

piranna · 2016-08-14T20:07:38Z

this module is locked to the release schedule of Express.js itself

That's totally insane, being so much specific it should have its own lifecycle, specially for this backward compatible things... And if not, semantic versioning is just for that...

I'll try to circle back to this PR another day when I have some time to discuss a new API :)

Ok, send me an email when you have time to discuss it.

dougwilson · 2016-08-14T20:19:00Z

That's totally insane, being so much specific it should have its own lifecycle

You're always welcome to use another module or fork this module. This is not up for discussion, I'm sorry. This code could have easily just been embedded into Express core. It is broken out for you to use it without Express, not to have a different release schedule, I'm sorry this is an inconvenience to you. We barely have enough people to maintain all this module, let alone for them all yo have different release timelines,meaning we have to create multiple lineages for modules when security vulnerabilities come out.

I feel like this is getting a little too headed foe me, so I'm not longer going to participate in this PR for time time being. Another member of the organization is welcome to pick up moving this PR forward...

piranna · 2016-08-14T21:59:12Z

I'm currently using a fork of this repo, the point of willing my changes to be merged is to clean up and have a consistent repo, but just by suggesting to use a repo, although acceptable, only contribute to degrade npm ecosystem. On NodeOS we lack a lot of people too, being so picky with the pull-requests just makes everybody losing their time and willing to not contribute anymore. Everybody lose :-(

dougwilson · 2016-08-14T22:27:15Z

Your PR is just never going to land, as you have not even hinted that you are willing to make any changes to your PR, and our philosophies for maintain seems to be too different: You suggest that I just merge anything without question, and I only want to merge quality code. These are unfortunately mutually exclusive. If you want to get a feature added, please work to add the feature with the API we have, rather than debate which API style is better.

piranna · 2016-08-14T22:33:17Z

I did all the changes that you tell me to do. You are not looking for quality code, but a clean and verbose commits history. If you really wanted to have quality code, you'll start by thinking about changing the API, but I only hinted about that and tried to adapt to the current code style and project organization. If you are not willing to maintain this project is ok, just give me permissions and I'll maintain it in your place.

dougwilson · 2016-08-14T22:35:40Z

I did all the changes that you tell me to do.

I'm not sure why you say this, as there are many, many open comments on your code above, and no commits since any of them. No changes I can see.

just give me permissions and I'll maintain it in your place

No.

closes pillarjs#89 closes pillarjs#116

piranna mentioned this pull request Dec 10, 2015

Index as function #88

Closed

dougwilson self-assigned this Jan 16, 2016

dougwilson added the pr label Jan 16, 2016

dougwilson force-pushed the master branch from 8eac491 to 14604b4 Compare January 23, 2016 20:59

dougwilson added the ideas label Mar 6, 2016

dougwilson reviewed May 2, 2016
View reviewed changes

piranna added 3 commits August 13, 2016 14:05

Better usage of normalizeList()

fb8c0e7

Allow to use a function to generate the index

90e8f98

Test to allow a function to generate directory index

fb27471

piranna force-pushed the index branch from 38a5056 to cee9a3d Compare August 13, 2016 12:11

piranna added 4 commits August 13, 2016 18:00

Add index event and use it instead of overloading this._index att…

77bd35d

…ribute

Fixed linting

b1491ca

Removed trash code

1e688d6

Allow to set index function as option & increased coverage to 100%

f4c1476

piranna force-pushed the index branch from 166fb41 to f4c1476 Compare August 13, 2016 16:00

Use typeof instead of instanceof

6d2600c

dougwilson reviewed Aug 14, 2016
View reviewed changes

dougwilson closed this Aug 14, 2016

pillarjs locked and limited conversation to collaborators Aug 14, 2016

spaceguy101 pushed a commit to spaceguy101/send-throttle that referenced this pull request Jun 23, 2018

docs: add custom directory index view example

da7df19

closes pillarjs#89 closes pillarjs#116

Uh oh!

index event #89

index event #89

Uh oh!

Conversation

piranna commented Dec 10, 2015

Uh oh!

piranna commented May 2, 2016

Uh oh!

dougwilson May 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piranna commented Aug 13, 2016

Uh oh!

dougwilson commented Aug 13, 2016

Uh oh!

piranna commented Aug 13, 2016

Uh oh!

piranna commented Aug 14, 2016

Uh oh!

dougwilson commented Aug 14, 2016

Uh oh!

piranna commented Aug 14, 2016 • edited by dougwilson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piranna commented Aug 14, 2016

Uh oh!

dougwilson commented Aug 14, 2016

Uh oh!

dougwilson Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

piranna Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

dougwilson Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

dougwilson Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

dougwilson Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

piranna Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

dougwilson Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

dougwilson commented Aug 14, 2016

Uh oh!

piranna commented Aug 14, 2016

Uh oh!

dougwilson commented Aug 14, 2016

Uh oh!

piranna commented Aug 14, 2016

Uh oh!

dougwilson commented Aug 14, 2016

Uh oh!

piranna commented Aug 14, 2016

Uh oh!

dougwilson commented Aug 14, 2016

Uh oh!

piranna commented Aug 14, 2016

Uh oh!

dougwilson commented Aug 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`index` event #89

`index` event #89

dougwilson May 2, 2016 •

edited

Loading

piranna commented Aug 14, 2016 •

edited by dougwilson

Loading