Do not download vendor JS files in released plugin versions #1025
Conversation
There was a problem hiding this comment.
I'm not going to pretend I'm not hopelessly lost trying to understand some of the Bash wizardry here. A bit worrying from maintainability perspective, but then again, probably not a permanent solution I'd imagine. Ideally we'd move off the nightlies. I think we can already for TinyMCE. React 16's latest alpha was released back in February, so not as promising there.
Should we add some of the temporary files to gitignore in case the script is aborted or otherwise exists mid-execution? Thinking *.tmp.js.
bin/build-plugin-zip.sh
Outdated
| php bin/get-vendor-scripts.php debug | ||
| ) | ||
| while IFS='|' read -u 3 url filename; do | ||
| wget -nv "$url" -O "vendor/$$.tmp.js" |
There was a problem hiding this comment.
wget is not stock Mac friendly:
./bin/build-plugin-zip.sh: line 41: wget: command not found
Maybe just curl: https://stackoverflow.com/a/4572158
There was a problem hiding this comment.
TIL $$, but also TIL apparently it's unwise for temporary files?
https://stackoverflow.com/questions/78493/what-does-mean-in-the-shell
There was a problem hiding this comment.
The issues with $$ and other "weak" methods of temporary filename generation are related to privileged processes that can be tricked into writing unexpected files that a user would not otherwise be able to write to. Thankfully, this set of issues does not apply to us.
Still, I've removed the use of $$ to avoid promoting this practice.
bin/build-plugin-zip.sh
Outdated
| # Run the build | ||
| npm install | ||
| npm run build | ||
|
|
||
| # Remove any existing zip file | ||
| rm -f gutenberg.zip | ||
|
|
||
| # Temporarily modify `gutenberg.php` with production constants defined | ||
| php bin/generate-gutenberg-php.php > gutenberg.$$.php |
There was a problem hiding this comment.
Can we write directly into gutenberg.php here?
There was a problem hiding this comment.
I wouldn't recommend it, since the php script is reading line-by-line from the same file at the same time. This might work due to kernel buffering or something like that, but it seems like undefined behavior to me. Added a comment to clarify.
nylen
force-pushed
the
add/vendor-js-to-plugin-zip
branch
from
9f9d49b
to
7be1f75
Compare
I added a couple more comments to try to address this - but yes, I agree this is not a permanent solution, and the way we handle vendor scripts needs some reworking. Nightlies are part of the issue, our current setup does what we need it to do without having to be configured, but could probably be redesigned a bit.
Everything in |
nylen
force-pushed
the
add/vendor-js-to-plugin-zip
branch
from
7be1f75
to
67d6c42
Compare
|
I'd like to merge this today so that we can move forward with a release to the plugin directory ahead of WCEU. |
There was a problem hiding this comment.
Should I expect that this downloads both minified and unminified versions of dependencies?
https://unpkg.com/react@next/umd/react.production.min.js
> vendor/react.min.c7397a88.js ... done!
https://unpkg.com/react-dom@next/umd/react-dom.production.min.js
> vendor/react-dom.min.912dc6cf.js ... done!
https://unpkg.com/react-dom@next/umd/react-dom-server.production.min.js
> vendor/react-dom-server.min.538e07c8.js ... done!
https://unpkg.com/[email protected]/min/moment.min.js
> vendor/moment.min.0fc04ee9.js ... done!
https://fiddle.azurewebsites.net/tinymce/nightly/tinymce.min.js
> vendor/tinymce.min.04a1d751.js ... done!
https://fiddle.azurewebsites.net/tinymce/nightly/plugins/lists/plugin.min.js
> vendor/tinymce-plugin-lists.min.e95f0005.js ... done!
https://unpkg.com/react@next/umd/react.development.js
> vendor/react.3a8927d9.js ... done!
https://unpkg.com/react-dom@next/umd/react-dom.development.js
> vendor/react-dom.e5cfcfc9.js ... done!
https://unpkg.com/react-dom@next/umd/react-dom-server.development.js
> vendor/react-dom-server.e84b9fa6.js ... done!
https://unpkg.com/[email protected]/moment.js
> vendor/moment.7eefd5c2.js ... done!
https://fiddle.azurewebsites.net/tinymce/nightly/tinymce.js
> vendor/tinymce.c847083c.js ... done!
https://fiddle.azurewebsites.net/tinymce/nightly/plugins/lists/plugin.js
> vendor/tinymce-plugin-lists.557da104.js ... done!
| function print_production_defines() { | ||
| global $plugin_version; | ||
|
|
||
| echo "define( 'GUTENBERG_VERSION', '$plugin_version' );\n"; |
There was a problem hiding this comment.
Can you explain why we want these defines? They don't appear to be used? Is it just because we need to replace the development mode constant so we might as well? Wondering if it'd help simplify this at all if we just moved to stripping out the define section during packaging.
There was a problem hiding this comment.
Is it just because we need to replace the development mode constant so we might as well?
There are a couple of other reasons as well:
- This is a simpler approach to Use get_plugin_data for production script versioning (cachebust) #440. See Add asset cache version helper function. #585 for another way to do it which I would not be as comfortable merging.
- Tying each plugin release to a particular git commit is a good idea to make sure that we can reliably identify where each build came from. This should also be done using git tags, for example, but a bit of redundancy here is a good thing.
|
Observing persistent 500 error after trying to activate plugin generated by this branch on a test site. Will follow-up when I learn more. Edit: Appears to be because the plugin is trying to |
bin/build-plugin-zip.sh
Outdated
| while IFS='|' read -u 3 url filename; do | ||
| echo "$url" | ||
| echo -n " > vendor/$filename ... " | ||
| curl -s "$url" -o "vendor/_download.tmp.js" |
There was a problem hiding this comment.
I think we need to follow redirects here (-L).
Finding I have vendor dependencies files written with the content like the following:
<p>You are being redirected to <a href="/[email protected]/umd/react-dom.development.js">/[email protected]/umd/react-dom.development.js</a>
There was a problem hiding this comment.
Indeed, I am not sure how I missed this 😬 Fixed in latest push.
Yes, so that we respect (at least partly) the |
nylen
force-pushed
the
add/vendor-js-to-plugin-zip
branch
from
791d72a
to
171f9c5
Compare
Fixed in #1095 and |
mtias
added
[Type] Build Tooling
|
Thanks for working on this one. |
|
Thanks for reviewing. I've been thinking about how to simplify this, and probably (when we decide we're ready to stabilize dependency versions a bit) we can do something like this:
We would lose the current behavior where we automatically download new versions of these dependencies every day, so we probably shouldn't do this until we are reasonably confident in our versions of React and TinyMCE in particular. |
This PR introduces the first example of logic needing to vary between the development and release versions of the plugin. In development mode, we download and cache "nightly" vendor JavaScript files, but this isn't appropriate for release - we should bundle these with the plugin instead.
This is accomplished by generating a slightly different
gutenberg.phpfile during the plugin release process, then only downloading vendor files if theGUTENBERG_DEVELOPMENT_MODEconstant is truthy.We respect the
SCRIPT_DEBUGconstant for these vendor scripts, but not yet for our own JavaScript. I'll create a separate issue for this (#1026).Follow-up to #985.
Addresses #953 (comment), after this we should be ready to release Gutenberg to the WP plugins directory.
Fixes #921 at the same time, since I was making changes in that area.