PublicLab.Editor

PublicLab.Editor copied to clipboard

Fixes #607

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

• [ ] tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
• [ ] code is in uniquely-named feature branch and has no merge conflicts
• [ ] PR is descriptively titled
• [ ] PR body includes fixes #0000-style reference to original issue #
• [ ] ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@jywarren @sagarpreet-chadha @keshav234156 @shreyaa-sharmaa @NitinBhasneria please do check this out. Travis isn't passing still and is returning the same error as with the previous test failures:

ReferenceError: Can't find variable: PL in file:///home/travis/build/publiclab/PublicLab.Editor/spec/javascripts/button_spec.js (line 7) (1) Expected false to be true. (2)

Do we need to change the es version in jshint here

https://github.com/jywarren/woofmark/blob/39237b798308e6855acde1a938c18a8e4da62fce/.jshintrc#L2

to es5 @sagarpreet-chadha ?

This is the grunt config file: https://github.com/publiclab/PublicLab.Editor/blob/main/Gruntfile.js

Wait though, do the Jasmine tests run on the compiled version? I guess they do, ok... I'll accept the suggestions above to see if they work even without teaching Grunt about babel!

OK just to review what's happening here:

before_script
6.21s$ npm run build
2.18s$ grunt jasmine --force
Running "jasmine:publiclabeditor" (jasmine) task
Testing Jasmine specs via PhantomJS
>> TypeError: Attempted to assign to readonly property. at
>> dist/PublicLab.Editor.js:2324 
>> dist/PublicLab.Editor.js:2322 
>> dist/PublicLab.Editor.js:2323 
>> dist/PublicLab.Editor.js:1 s
>> dist/PublicLab.Editor.js:1 
>> dist/PublicLab.Editor.js:2365 
>> dist/PublicLab.Editor.js:1 s
>> dist/PublicLab.Editor.js:1 e
 Publish button
   - Publish button is enabled...X Publish button is enabled
     ReferenceError: Can't find variable: PL in file:///home/travis/build/publiclab/PublicLab.Editor/spec/javascripts/button_spec.js (line 7) (1)
     Expected false to be true. (2)

I believe this is an issue with PhantomJS - that errors are read-only?

https://stackoverflow.com/questions/20055368/typeerror-attempted-to-assign-to-readonly-property

Hmm. The line is:

this.Class=function(){};// Create a new Class that inherits from this class -- so maybe not an error related issue...

Aha - this occurs in strict mode!

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Read-only

And this.Class=function(){};// Create a new Class that inherits from this class is from the resig-class dependency! https://johnresig.com/blog/simple-javascript-inheritance/

https://www.npmjs.com/package/resig-class -- can we find an alternative that works with strict mode/ES6?

Used in all these files: https://github.com/publiclab/PublicLab.Editor/search?q=resig&unscoped_q=resig

Some possible related research: https://stackoverflow.com/questions/15050816/is-john-resigs-javascript-inheritance-snippet-deprecated

wow this is obscure!

Made an issue here on the NPM module - https://github.com/mattinsler/resig-class/issues/2

OK! I tried replacing resig-class with the one modified for strict mode in https://stackoverflow.com/a/15052240

But now get the error:

Running "jasmine:publiclabeditor" (jasmine) task
Testing Jasmine specs via PhantomJS

>> ReferenceError: Strict mode forbids implicit creation of global property 'PL' at
>> dist/PublicLab.Editor.js:2361 

If i skip the babel step, i get the original issue:

Running "jasmine:publiclabeditor" (jasmine) task
Testing Jasmine specs via PhantomJS

 Publish button
   X Publish button is enabled
     ReferenceError: Can't find variable: PL in file:///workspace/PublicLab.Editor/spec/javascripts/button_spec.js (line 7) (1)
     Expected false to be true. (2)

So, I think we may need to configure babel to not use strict mode? Ah - wait - babel can be configured for script mode and then doesn't use strict: https://stackoverflow.com/a/34983495 (if i understand this properly... 😅 )

OK, so i added:

  "sourceType": "script",

to .babelrc, and got:

>> TypeError: undefined is not a function (evaluating 'Class.extend') at

Aha! I reverted the resig-class back and now it works! Let me try here.

Fingers 🤞 !!!

This has been a really deep obscure issue! Whoa!!! 🤯

OMG lol. Jasmine tests now run. But Jest tests fail:

> [email protected] test-ui /home/travis/build/publiclab/PublicLab.Editor
> node ./node_modules/.bin/jest
 FAIL  test/ui-testing/CustomInsert.test.js
  ● Test suite failed to run
    ReferenceError: regeneratorRuntime is not defined
      1 | const timeout = process.env.SLOWMO ? 60000 : 10000;
      2 | const fs = require('fs');
    > 3 | beforeAll(async () => {
        |          ^
      4 |   path = fs.realpathSync('file://../examples/index.html');
      5 |   await page.goto('file://' + path, {waitUntil: 'domcontentloaded'});
      6 | });
      at Object.<anonymous> (test/ui-testing/CustomInsert.test.js:3:10)

Can we do this in another order?

Wow - the folks at https://github.com/mattinsler/resig-class/issues/2 just updated to v2.0.0 with a change that may make this error no longer occur. Let's try reverting the "sourceType": "script", back and see what happens... 😅

OK - so, we now see this again:

>> ReferenceError: Strict mode forbids implicit creation of global property 'PL' at
>> dist/PublicLab.Editor.js:2361 

I think we can get past this if we turn on "script" mode in babel once more. But that will still break the Jest tests. But I think we actually do need babel to be compiling for script mode... i'm going to turn that back on and we will probably need to adjust the Jest test setup to get those running again. Sorry for the run-around here! Looks like we're back to where we were on Tuesday!

Aha searched around for this error "Jest ReferenceError: regeneratorRuntime is not defined" - here is the issue, i think! https://github.com/facebook/jest/issues/7579#issuecomment-453040736

it's advised to use @babel/plugin-transform-runtime along with @babel/runtime to support async iterators with Babel instead of regenerator-runtime if you're on Babel7 and Jest 23?

The next comment down has another suggestion which I'll try:

// babel.config.js
module.exports = {
    presets: [
        ['@babel/preset-env', { targets: { node: 'current' } }],
    ],
}

@jywarren sorry i've been away here. I've not been able to quickly wrap my head around what should be done next. :smile:

I'm not sure if this makes any different but i see that the example here https://stackoverflow.com/a/34983495 is setting sourceType to script in Babel 7's overrides. like this:

overrides: [{
 test: "./vendor/something.umd.js",
 sourceType: "script",
}],

Does this matter here? And another option is to to set sourceType to "unambiguous"

Thanks @Shulammite-Aso -- no problem, i'm just trying to crack this tough one! I just made a syntax error so i'll fix that and see if it works. But if it doesn't, either of your two ideas are probably worth trying! And also, probably easier to iterate faster in GitPod or locally, instead of waiting for Travis to run as I'm doing 😅

Hmm:

Error: [BABEL] /home/travis/build/publiclab/PublicLab.Editor/dist/PublicLab.Editor.js: Unknown option: .targets. Check out https://babeljs.io/docs/en/babel-core/#options for more information about options.
- Maybe you meant to use
"preset": [
  ["@babel/preset-env", {
  "targets": {
    "node": "current"
  }
}]
]

Hmm, ok, we're back to:

     ReferenceError: Can't find variable: PL in file:///home/travis/build/publiclab/PublicLab.Editor/spec/javascripts/button_spec.js (line 7) (1)

@Shulammite-Aso would you like to try a few variations?

I think it may be worth checking if we need @babel/plugin-transform-runtime although perhaps we've gotten past that issue now? Or, could we do an override of strict mode for just our code, though we leave it in place for the dependencies? Not sure how this works, but let's try a few solutions. Thank you!

Hmm -- unfortunately it doesn't look like it worked. Would anyone from the broader @publiclab/editor team want to try to get this to pass? Thanks, everyone!

Hey, great work @Shulammite-Aso! I'm looking into it, will let you know if I find something relevant to this issue. Great collaboration everyone! :tada: :tada:

I will also try this 👍

Took latest fork of repo, getting ReferenceError: beforeAll is not defined while running jest tests

Hmm, in relation to this and similar lines?

https://github.com/Shulammite-Aso/PublicLab.Editor/blob/4f8acd7121da788f0610657aac3133f47b193c9b/test/ui-testing/CustomInsert.test.js#L3

Has this somehow been affected by babel? Is it possible to run jest locally or in GitPod?

Thanks @Shulammite-Aso @shreyaa-sharmaa @Sagarpreet! Ping me in the chatroom if you get really stuck and I can try working on it locally or in gitpod for a bit! Thanks!!!

I get Can't find variable: PL on the latest fork. What about you @Shulammite-Aso ?

Hmm, can't run this in Gitpod as its missing puppeteer...