-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
File System API (Final On-Hold) See Comments #786
Conversation
webpack.dev.js
Outdated
@@ -61,6 +61,6 @@ module.exports = merge.smart(require('./webpack.base.js'), { | |||
historyApiFallback: true, | |||
disableHostCheck: true, | |||
headers: {"Access-Control-Allow-Origin": "*"}, | |||
setup: fileSystemAPIPlugin, | |||
before: fileSystemAPIPlugin, |
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.
@tech4him1 good catch, I ran this for the last 4 days and did not notice the warning 👍
example/assets/preview/config.yml
Outdated
backend: | ||
name: github | ||
repo: netlify/netlify-cms | ||
branch: cms-test-backend |
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.
If we use the CMS repo on the GitHub backend for deploy previews, contributors that do not have write permissions will not be able to use our deploy previews. Right?
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.
True. That branch is an orphan branch we setup to test the backend, so in most cases we would just have to give those access rights to contributors that need them.
Do you think this is an issue? Maybe hard to maintain?
On another note: We could always lock files in the paths for production and this one to make sure there is not an overwrite. Not sure we want that, but an idea.
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.
@erquhart can you let me know if what Caleb is asking here is an issue, so we can address?
The solution is fine either way, just want to make sure we don't introduce any issues on deploy previews.
36593a3
to
eae6442
Compare
There has been a rebase to version |
3f61e96
to
45d26e5
Compare
const fileURL = `/file/${ path }`; | ||
return this.request(fileURL, { | ||
method: "DELETE", | ||
params: { }, |
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.
These params: { },
should be necessary.
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.
right now there is nothing being done with them to the file-system, but maybe later in the case we want to do logging etc in next phase.
@@ -26,7 +26,7 @@ function nameFromEmail(email) { | |||
export default class TestRepo { | |||
constructor(config) { | |||
this.config = config; | |||
this.assets = []; | |||
this.assets = window.repoFiles.assets || []; |
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.
Can you explain why this line is needed for this PR?
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.
https://github.com/netlify/netlify-cms/pull/786/files#diff-1634bb1b7f4da7ca72cddcb74fef74b7R12
Allows for the loading of the assets that are truly in the assets/uploads
folder. The way they would show up if you were truly hitting a real api.
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.
Awesome!
8ab4b03
to
0ed46ba
Compare
I have just tested on macOS and this seems to be in working order. I for one cannot wait for this to land. |
Awesome, thanks for testing it out @jrop! We definitely want to get this in soon. |
I also fired this up in my Ubuntu VM and creating a post worked fine. I don't know what kind of testing you guys are looking for, though. |
Thanks @jrop Update: Rebased to version 0.7.6 , so make sure when pulling this branch, you pull a clean checkout from origin. |
0ed46ba
to
f4c2f20
Compare
@talves - Do you have a guess when this will land? I'm very interested in using it on a locally hosted server. Thanks! |
@AstroJetson I am only a contributor, so if it is accepted @erquhart will have the details on when it will land. I believe it will get looked at after 1.0 is my best guess. |
@talves I'm pretty beat from the 1.0 rush, but I'm planning onto digging into this next week. From what I've seen it looks like a massive improvement over the in-memory repo 🎉 |
added a _sink example
Deploy preview for netlify-cms-www ready! Built with commit 70640c8 |
Deploy preview for cms-demo ready! Built with commit 70640c8 |
Rebased as of 01/13/2018 Important Notes:
|
@talves you mentioned discussing use of this as an external backend - #1011 has landed, so it's possible. We'll be converting to Lerna and keeping "official" extensions in the repo at that point, of which this would definitely be one. I'm personally in favor of moving this (along with every backend except test-repo) into a separate package and keeping the core repo slim, but we should definitely discuss. Thoughts? cc/ @tech4him1 @Benaiah |
There was a lot of discussion on bringing the current internal backends out "into the wild" between me and @tech4him1. There are a lot of considerations and we might have thought it would be better to keep the current backends in place until we hash out a lot of these considerations. Things to consider:
These are all obvious, but really should be discussed to keep the CMS focused (github) I have the localhost already completed and will publish it soon (very soon). This alone will help us to spark conversation on if we want to keep localhost fs internal with github, but make others external. Let the discussion begin. 😁 |
Here is the backend repository for the file-system in this PR Checkout the Eventually this repository will be put into the cms as a mono-repo. I will keep this PR open for discussion of that repository since it will be replacing this PR. |
Should we Close ths PR or Leave it (mark on-hold) for reference until we have a viable mono-repo for the complete solution? |
@talves if I'd like to test the local backend for development, am I right to assume that I go to the aforementioned repo and try it from there? Will be using it with Gatsby … I guess there's no way of just setting the backend option in config.yml just yet? |
@bjrn yes, you can easily clone/fork my repo and run a build. You can always hit me up in Gitter if you need help setting it up. The readme instructions has most of what you need to wire it into your project. Eventually this repo will be a mono-repo inside the CMS and will be accessible through npm, but is fully functional and tested for local development to use now if you want in a project. |
Closing as stale. @talves deleting branch as the it's backed up on your fork. |
@erquhart No need to back up. It has it's own repo(s) in my account. @jrop Exists for v1.x. See this example repo. I will more than likely move it for v2.x when I get some time. You could always take a stab at it. Not sure if we will keep it as a community driven backend or move it into the repo, because it needs so much extra setup on top of a project to spin up the File-System API. |
Would it make sense to maintain the CMS backend separately from the node server? The backend could then be more of a generic HTTP backend, and any service that implements the REST endpoints can then be dropped in. Separately, the node filesystem server just implements the rest interface. |
As motivation for this idea, I've been using the CMS backend portion of this for a while now in Orchid. Orchid has a development server that implements the api needed by the backend, and so local development goes through the Orchid server, rather than the node server. |
@cjbrooks12 these endpoints are not part of the file-system backend, they just happen to be setup in a server using node in the example repo. You could use any server as long as you setup the same endpoints. Are you suggesting something different? Maybe I am misreading your suggestion. |
Right, the endpoints and their formats are just what the Node server happens to be spitting out, but if we invert how we think of this, the "filesystem backend" is just a backend that hits a REST API at a given URL, and the actual filesystem portion just happens to implement the correct API endpoints with data from the local filesystem. But because the CMS backend is just making HTTP calls, any server that produces the same response can be used and the CMS will work just fine, which is how I've already been using it for a while. I've tossed aside the actual "filesystem" portion of your fork, the node server, and replaced it with Orchid's own development server. Orchid then re-implements (in Kotlin) the code necessary to transform its own concept of pages on disk into the JSON response that the CMS backend is expecting. Conceivably, if the actual format that the CMS backend reads were to be formalized and documented, other tools could create ways to implement those endpoints in their own development servers. Think, a Gatsby plugin that allows you to run |
I second the idea to have a backend creation "spec" or "guide". I would potentially like to create my own backends for Netlify CMS. |
Note: This PR is on hold, because the solution will be moved to an outside proposed mono-repo. See this repo for a working solution.
- Summary
Create an end point file system api middleware for the express server (webpack-dev-server) to be able to create a backend api in the CMS to update the example site locally.
yarn
andyarn start
Example on windows:
Closes #329
Closes #710
Closes #706
- Test plan
- Description for the changelog
Features:
/api
path at two endpoints/api/file/:id
and/api/files/:path
@Benaiah @erquhart I will explain as much as I can on this PR, so you can test quicker.
I have created a new example structure at the root of
example
the way Shawn likes it 😄 It has the same paths and folders as the orphan branchcms-test-backend
.Structure for deploy:
I created a
config.yml
file in the assets folder to use when a deploy preview kicks off from now on that will point to the orphan branch. All someone has to do on a test branch that needs a different layout is put their copy of the config in there and it will now get copied over to the rootexample/config.yml
when the PR deploy preview is created.API
The new backend API is called
file-system
in the cms. It targets the endpoints in the API middleware in thescripts/fs
folder. When a webpack dev server is started, the api routes are added to the server.Enhancements (updated: 11-8-2017)
production/config.yml
andproduction/index.html
on aproduction
deploy (these should not change except on design changes)preview/config.yml
on abranch-deploy
anddeploy-preview
(these are ok to change for your branch targets)Notes:
I think all the backend API's should be plugins at some point for the cms, so we are not carrying around unnecessary code.
The failures on read or write do make the cms fail elegantly. Any errors will show up in the inspect of the page as 500 errors.
Hope this all makes sense and code like you are on 🔥
I will close #781 due to conflicts on my rebase of the branch.