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

Fix TTW Bundle compilation broken. #3011

Merged
merged 1 commit into from
Jan 4, 2020
Merged

Fix TTW Bundle compilation broken. #3011

merged 1 commit into from
Jan 4, 2020

Conversation

thet
Copy link
Member

@thet thet commented Jan 3, 2020

Fixes #2969
Fixes plone/plone.staticresources#58

The PR #2970 got merged for master, which became Plone 6.0 during the Ploneconf sprints IIRC.

@mister-roboto
Copy link

@thet thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@davilima6
Copy link
Member

@jenkins-plone-org please run jobs

@davilima6 davilima6 self-requested a review January 3, 2020 17:54
@davilima6
Copy link
Member

davilima6 commented Jan 3, 2020

Code LGTM as it's the same already approved in #2970. Just a curiosity, why was the error handling removed? I'll test it now.

@davilima6 davilima6 requested a review from mtrebron January 3, 2020 17:57
@davilima6
Copy link
Member

@thet, I still get the error on buildout.coredev after changing to thet-2969-5.2.x branch of src/Product.CMFPlone, starting the instance and creating a fresh site. Am I missing some step?

@davilima6
Copy link
Member

davilima6 commented Jan 3, 2020

Actually the error now is a bit different. Before it was:

GET http://localhost:8080/Plone2/++plone++static/components/r.js/dist/r.js net::ERR_ABORTED 404 (Not Found)
Uncaught TypeError: Cannot read property 'optimize' of undefined
    at Object.onLoad (resourceregistry-compiled.js:33643)
    at window.IFrame.load (resourceregistry-compiled.js:33406)

Now the first error is different:

VM360 r.js:1 Uncaught SyntaxError: Invalid or unexpected token
resourceregistry-compiled.js:33643 Uncaught TypeError: Cannot read property 'optimize' of undefined
    at Object.onLoad (resourceregistry-compiled.js:33643)
    at window.IFrame.load (resourceregistry-compiled.js:33406)

Can you reproduce?

@davilima6
Copy link
Member

davilima6 commented Jan 3, 2020

It seems an unexpected shebang got prepended to the r.js file. Actually not so unexpected since it's in bin/ so supposed to have +x permission. Not sure how to proceed: editing the file manually would fix it but may not survive Npm upgrades.

#!/usr/bin/env node
/**
 * @license r.js 2.3.6 Copyright jQuery Foundation and other contributors.
 * Released under MIT license, http://github.com/requirejs/r.js/LICENSE
 */

/*
 * This is a bootstrap script to allow running RequireJS in the command line
 * in either a Java/Rhino or Node environment. It is modified by the top-level
 * dist.js file to inject other files to completely enable this file. It is
 * the shell of the r.js file.
 */

/*jslint evil: true, nomen: true, sloppy: true */
/*global readFile: true, process: false, Packages: false, print: false,
console: false, java: false, module: false, requirejsVars, navigator,
document, importScripts, self, location, Components, FileUtils */

var requirejs, require, define, xpcUtil;
(function (console, args, readFileFunc) {
    ...

@mtrebron
Copy link

mtrebron commented Jan 3, 2020

Looks good from here! FF 72.0b11 (Aurora) on Ubuntu.

@davilima6
Copy link
Member

davilima6 commented Jan 3, 2020

Update: Firefox (71) parser seems to be more permissive and despite some apparently unrelated "malformed XML" errors in console, TTW build works for me!

XML Parsing Error: junk after document element
Location: http://localhost:8080/Plone5/++resource++mockup/filemanager/templates/app.xml
Line Number 3, Column 1:

Newest Edge, Opera and Safari also fail on Mac, though, with the same SyntaxError as in Chrome.

@davilima6
Copy link
Member

Failures seem unrelated:

Error in test test_all_supported_languages (plone.app.multilingual.tests.test_setup.TestSetupMultilingualSite)
Traceback (most recent call last):
  File "/srv/python2.7/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/jenkins/.buildout/eggs/cp27m/plone.app.multilingual-5.5.0-py2.7.egg/plone/app/multilingual/tests/test_setup.py", line 49, in test_all_supported_languages
    setup_tool.setupSite(self.portal)
  File "/home/jenkins/.buildout/eggs/cp27m/plone.app.multilingual-5.5.0-py2.7.egg/plone/app/multilingual/browser/setup.py", line 79, in setupSite
    doneSomething += self.setUpLanguage(language, name)
  File "/home/jenkins/.buildout/eggs/cp27m/plone.app.multilingual-5.5.0-py2.7.egg/plone/app/multilingual/browser/setup.py", line 152, in setUpLanguage
    self.context[folderId], assets_folder_id)
  File "/home/jenkins/workspace/pull-request-5.2@4/src/Products.CMFPlone/Products/CMFPlone/utils.py", line 351, in _createObjectByType
    id = str(id)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-5: ordinal not in range(128)



Error in test test_type_of_language_folders (plone.app.multilingual.tests.test_setup.TestSetupMultilingualSite)
Traceback (most recent call last):
  File "/srv/python2.7/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/jenkins/.buildout/eggs/cp27m/plone.app.multilingual-5.5.0-py2.7.egg/plone/app/multilingual/tests/test_setup.py", line 69, in test_type_of_language_folders
    setup_tool.setupSite(self.portal)
  File "/home/jenkins/.buildout/eggs/cp27m/plone.app.multilingual-5.5.0-py2.7.egg/plone/app/multilingual/browser/setup.py", line 79, in setupSite
    doneSomething += self.setUpLanguage(language, name)
  File "/home/jenkins/.buildout/eggs/cp27m/plone.app.multilingual-5.5.0-py2.7.egg/plone/app/multilingual/browser/setup.py", line 152, in setUpLanguage
    self.context[folderId], assets_folder_id)
  File "/home/jenkins/workspace/pull-request-5.2@4/src/Products.CMFPlone/Products/CMFPlone/utils.py", line 351, in _createObjectByType
    id = str(id)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-5: ordinal not in range(128)

@thet
Copy link
Member Author

thet commented Jan 4, 2020

@davilima6 I'll restore the r.js and less.js files in the versions we were using until plone.staticresources - including the patches applied like mentioned here: https://github.com/plone/plone.staticresources#warning
I'll provide these resources out from the npm upgrade process in a seperate directory which also includes an upgrade step. TTW compilation - IIRC - will only be available until Plone 6 or so. requirejs should go away in favor of standard ES6 imports.

I already started but will finish later this weekend.

This PR should be fine to merge, though.

@davilima6 davilima6 merged commit 5fd285e into 5.2.x Jan 4, 2020
@davilima6 davilima6 deleted the thet-2969-5.2.x branch January 4, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants