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

Changes to API #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Changes to API #4

wants to merge 1 commit into from

Conversation

stefanoverna
Copy link

@stefanoverna stefanoverna commented Jul 15, 2017

And here's an example of usage: https:/stefanoverna/spike-datocms-example/blob/master/app.js

The main changes:

Copy link
Member

@jescalan jescalan left a comment

Choose a reason for hiding this comment

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

So it looks like the tests are failing here still, but definitely understand what you're after in general and want to get this merged 👍

}
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Very clever way of making this happen! Curious if you had any other thoughts working with reshape and its API

Copy link
Author

@stefanoverna stefanoverna Jul 19, 2017

Choose a reason for hiding this comment

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

That was an hack actually 😛 I'm not using reshape to generate the HTML, resorting to some external helper. I don't think that's very advisable... it would be better to return some reshape AST nodes instead, so that they could then be further processed by other reshape plugins down the chain (ie. reshape/minify). I could not find a way to do that though... 🆘


// if cache is set and cache file doesn't exist; write it
if (this.cache && this.rewrite) {
writeCache(this.cache, this.addDataTo.dato)
Copy link
Member

Choose a reason for hiding this comment

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

why remove the cache feature?

Copy link
Author

Choose a reason for hiding this comment

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

Well, DatoCMS pushes some websocket notifications everytime the content changes:

const SiteChangeWatcher = require('datocms-client/dump/SiteChangeWatcher');
const watcher = new SiteChangeWatcher(site.id);
watcher.connect(function() { 
  console.log('Data changed!'); 
});

So I think we should definitely get rid of the cache option, and automatically cache between these notifications.

@@ -34,61 +29,21 @@ module.exports = class SpikeDatoCMS {
})

compiler.plugin('emit', (compilation, done) => {
if (this.json) {
writeJson(compilation, this.json, this.addDataTo.dato)
}
Copy link
Member

Choose a reason for hiding this comment

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

Also removing the writeJson feature?

Copy link
Author

Choose a reason for hiding this comment

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

that was just temporary, I can re-enable it, but I thought that was redundant, as you can use the the singlePages option to do the same. Well, not at the moment, because the writeTemplate is not using webpack, right?

@stefanoverna
Copy link
Author

@jescalan if you like the overall direction of the proposed changes, I can finalize things and write tests. Let me know if I can start :)

@jescalan
Copy link
Member

@stefanoverna Yeah, I would say don't worry about the meta tags helper, I can take that section, but as far as the primary piece with using the client, I think this is good to start testing!

Interesting with the websockets though, this is really cool. I can also hook this up to the watcher, don't worry about that part.

@stefanoverna
Copy link
Author

Hi @jescalan, we're receiving requests regarding this PR on our support chat.. looking forward to close it! 😃 I'm not very familiar with the internals of spike so I would really need some help of yours :)

screenshot 2017-08-29 12 30 27

@jescalan
Copy link
Member

So, this is massively breaking and still has no tests at all, so I can't really merge it yet, it still needs a bunch of work. Also, what's being asked there can already be accomplished with the plugin as it exists - you're welcome to direct them here to the issues section in this repo with questions!

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.

2 participants