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

WIP: Refactoring #3

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

BigBlueHat
Copy link
Collaborator

This one goes on top of host-out-of-www (once/if that's merged).

The objective is smaller bits of more easily testable/understandable/reusable code.

The goal is to end up with three chunks of code: server, client, and cli. The cli is the command line interface to client. The other two do what they say on the tin. 😁

BigBlueHat added 17 commits May 31, 2017 10:51
Enforce local building of code--for now.
Prevent accidently over sharing credentials.
This provides for bootstrapping a hosting directory
with a minimal discovery HTML document.

It is copied on the first run of the server, but not
on subsequent runs to avoid overwriting changes.

Re-bootstrapping simply requires moving the `index.html`
from the `rootPath` directory.
rootPath is now...root. Other paths are all expected to
be served from inside of rootPath (relatively).
Avoids double // issues
This is a bit heavier refactoring than I like to do
in one go. getSerialization() function signature now
includes passing in rdfaTypes. Small smell...but we
can fix that later.
Mostly for future use by other projects
@mielvds
Copy link
Contributor

mielvds commented Oct 11, 2019

👍 for merging this @csarven, very much needed. For the server, I hope we end up with different express middleware that can be used separately to run inboxes as a library.

@csarven
Copy link
Owner

csarven commented Oct 11, 2019

I'm also +1 to this but IIRC the changes broke https://github.com/csarven/ldn-tests/ . I'd prefer to merge once ldn-tests factors in the changes in mayktso. [mayktso was initially created for ldn-tests and then pulled out.. I don't want to track two different mayktsos in production.]

@mielvds
Copy link
Contributor

mielvds commented Oct 11, 2019

Of course, but what is still needed for that? And https://github.com/csarven/ldn-tests/ should probably use this repo as a dependency/library?

@csarven
Copy link
Owner

csarven commented Oct 11, 2019

Besides bring this PR up to date with current master? ..

Off the top of my head, we need to be careful with the URL and file paths.

ldn-tests already includes maytkso in its package as a dependency.

@mielvds
Copy link
Contributor

mielvds commented Oct 11, 2019

ldn-tests already includes maytkso in its package as a dependency.
Ah misunderstood.

Maybe it's better to redo the refactoring on the current master with this PR as inspiration.

@BigBlueHat
Copy link
Collaborator Author

@csarven the conflicts don't seem that bad, actually. You can use the "Resolve conflicts" button here to preview (and manage them too, actually).

I'm not familiar enough with ldn-tests to know what I've broken (or not) about it. You mentioned URL and file paths. Are there enough tests in that package that could confirm (or deny) any refactoring success?

I'm happy to take another crack at this. 😃

@@ -0,0 +1,13 @@
module.exports = function(req, res, next) {
Copy link
Contributor

@mielvds mielvds Oct 14, 2019

Choose a reason for hiding this comment

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

@BigBlueHat can't this be replaced with https://www.npmjs.com/package/cors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably, but I'd recommend doing that as a separate PR. The goal of this PR was to simply refactor the existing code--not introduce any new dependencies, etc.

Once this PR is in, then yes, it would be great to reconsider adding dependencies.

@mielvds
Copy link
Contributor

mielvds commented Feb 17, 2022

@BigBlueHat Any plans to continu this?

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.

3 participants