wp-shortcake / shortcake-bakery

A fine selection of shortcodes for WordPress
42 stars 16 forks source link

Switch to gh-pages commit for reference #206

Closed davisshaver closed 7 years ago

davisshaver commented 7 years ago

screen shot 2017-01-25 at 3 04 35 pm

davisshaver commented 7 years ago

@goldenapples I don't really understand what's going on here. The commit referenced seems valid (https://github.com/mozilla/pdf.js/commit/04e2168f137cbb9c4cb3313441564a3549e6c1f9) but it's not the commit referenced in gh-pages history. https://github.com/mozilla/pdf.js/commits/gh-pages

What do you think?

goldenapples commented 7 years ago

Huh. Looks like the make script that builds the gh-pages branch in that repository doesn't preserve history; it just replaces the HEAD commit of that branch with each commit to the master branch.

What's the problem you're trying to solve here? Does checking out the older commit hash not work?

Oh, never mind. I see the error you're getting in the screenshot. Hmm.

davisshaver commented 7 years ago

@goldenapples That's annoying. Yeah if the commit vanishes, the plugin's broken as a submodule. Even checking out to gh-pages seems to be pointing to the latest commit behind the scenes.

goldenapples commented 7 years ago

So supposedly (haven't tried this yet though) git 1.8+ has the ability to reference a submodule by branch or tag, rather than by commit hash. Stack Overflow

I wonder if that will work here? Or if we'll have to just check all the files into this repo and give up on the submodule idea...

davisshaver commented 7 years ago

@goldenapples I tried it just as git checkout gh-pages but that SO link looks more involved. If it's doable seems better to structure as submodule.

davisshaver commented 7 years ago

@goldenapples Other than adding https://github.com/wp-shortcake/shortcake-bakery/pull/206/commits/cf9ae4b87f1e0b4bad8a40ac2a706c4d8462d239, I wasn't able to generate any kind of diff based on the SO tutorial. If you're able to get different results that'd be swell, but if not what do you think about checking the files in? Can put together a patch tomorrow if you're amenable.

goldenapples commented 7 years ago

I tried updating the submodule, after the change to the submodule branch configuration in cf9ae4b.

Looks like once that branch is defined in the submodule configuration, then running git submodule init; git submodule update --remote sets the submodule commit to the most recent commit on the gh-pages branch and then checks out that commit. So it seems like in theory if we don't check in the submodule commit hash, then this would work. We'd have to add instructions letting people know that they'd have to initialize the submodules with --remote.

But I don't like this much either, because then we don't have a way of locking the submodule to a specific known good commit.

what do you think about checking the files in? Can put together a patch tomorrow if you're amenable.

Yeah, I guess that's the only way to go here.

davisshaver commented 7 years ago

Holy diff, Batman! But on the upside, we wouldn't break in an automated build if PDF.js gets an update.

screen shot 2017-01-26 at 6 52 33 am
goldenapples commented 7 years ago

Seems to work fine for me, and since this is necessary to prevent breaking changes with every change to the upstream repo, I'd say we should get this merged in here.

For reference if anyone else wants to test out updating to this - I had to run git submodule deinit -f assets/lib/pdfjs before pulling in these commits.

I'd like to see some kind of documentation about how to update the pdfjs dependency - or even better, a build task that pulls down the latest commit from that repo, so that we don't have to look back through history to find what we did to get these files into the repo in the first place.

Other than that, the only thing I'd say is to remove the 1MB demo PDF file at assets/lib/pdfjs/web/compressed.tracemonkey-pldi-09.pdf, which we don't need for anything. (See https://github.com/mozilla/pdf.js/wiki/Setup-PDF.js-in-a-website#from-examples)

davisshaver commented 7 years ago

Adds a grunt dependencies command that pulls the latest zip from Github gh-pages branch and puts into the pdfjs folder. I also added some additional paths to .gitignore so we can slim down the version controlled bundle.

I notice the CI is failing, it looks some of the grunt-contrib- package I used haven't been updated for Grunt 1.x.

npm ERR! peerinvalid Peer grunt-contrib-clean@0.4.1 wants grunt@~0.4.0
npm ERR! peerinvalid Peer grunt-contrib-rename@0.0.3 wants grunt@~0.4.0
npm ERR! peerinvalid Peer grunt-sass@2.0.0 wants grunt@>=0.4.0
npm ERR! peerinvalid Peer grunt-wget@0.1.3 wants grunt@~0.4.0
npm ERR! peerinvalid Peer grunt-wp-i18n@0.5.4 wants grunt@>=0.4.0
npm ERR! peerinvalid Peer grunt-wp-readme-to-markdown@2.0.1 wants grunt@>=0.4.0

So that kind of sucks. I guess we wouldn't want to point grunt at 0.4.0? I wish we could just ignore the error...

goldenapples commented 7 years ago

I like it, seems to work well enough. We still have the lib version in the pdf.js file banner, so at least there's a way of checking to see what commit the lib is on.

I'm in favor of merging this so that it's possible to check out master branch here and have a functioning plugin.