Setup babel #614

Shulammite-Aso · 2020-09-13T21:24:43Z

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!

gitpod-io · 2020-09-13T21:24:46Z

Shulammite-Aso · 2020-09-13T23:34:41Z

@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 ?

jywarren · 2020-09-14T16:40:28Z

package.json

@@ -4,7 +4,8 @@
  "description": "PublicLab.Editor is a general purpose, JavaScript/Bootstrap UI framework for rich text posting, which provides an author-friendly, minimal, mobile/desktop (fluid) interface for creating blog-like content, designed for PublicLab.org",
  "main": "dist/PublicLab.Editor.js",
  "scripts": {
-    "build": "grunt build",
+    "build": "grunt build && ./node_modules/.bin/babel dist -d dist",


This looks great. Thank you @Shulammite-Aso !!!

I think perhaps we have to run grunt babel before grunt build or vice versa, in the travis config:

https://github.com/Shulammite-Aso/PublicLab.Editor/blob/2de7a7f703e7348fe4e320aa0ac7043780932565/.travis.yml#L5-L8

What do you think?

Yes @jywarren that sounds right
will just add that now!

grunt.initConfig({ babel: { options: { sourceMap: true, presets: ["@babel/preset-env"], }, dist: { files: { "dist/app.js": "src/app.js", }, }, }, }); grunt.loadNpmTasks('grunt-babel'); grunt.registerTask("default", ["babel"]);

I think we need to tell grunt as well that we have to use babel, more details here:
https://babeljs.io/setup#installation

package.json

sagarpreet-chadha · 2020-09-14T19:11:28Z

package.json

@@ -4,7 +4,8 @@
  "description": "PublicLab.Editor is a general purpose, JavaScript/Bootstrap UI framework for rich text posting, which provides an author-friendly, minimal, mobile/desktop (fluid) interface for creating blog-like content, designed for PublicLab.org",
  "main": "dist/PublicLab.Editor.js",
  "scripts": {
-    "build": "grunt build",
+    "build": "grunt build && ./node_modules/.bin/babel dist -d dist",


grunt.initConfig({ babel: { options: { sourceMap: true, presets: ["@babel/preset-env"], }, dist: { files: { "dist/app.js": "src/app.js", }, }, }, }); grunt.loadNpmTasks('grunt-babel'); grunt.registerTask("default", ["babel"]);

sagarpreet-chadha · 2020-09-14T19:12:05Z

package.json

@@ -4,7 +4,8 @@
  "description": "PublicLab.Editor is a general purpose, JavaScript/Bootstrap UI framework for rich text posting, which provides an author-friendly, minimal, mobile/desktop (fluid) interface for creating blog-like content, designed for PublicLab.org",
  "main": "dist/PublicLab.Editor.js",
  "scripts": {
-    "build": "grunt build",
+    "build": "grunt build && ./node_modules/.bin/babel dist -d dist",


I think we need to tell grunt as well that we have to use babel, more details here:
https://babeljs.io/setup#installation

sagarpreet-chadha · 2020-09-14T19:13:47Z

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

jywarren · 2020-09-15T16:25:58Z

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!

jywarren · 2020-09-15T16:28:44Z

.travis.yml

 script: 
+  - npm run build


i'm going ot try this as a before_script so we are sure it's done before it tries the tests.

jywarren · 2020-09-15T17:36:42Z

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...

jywarren · 2020-09-15T17:40:43Z

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/

jywarren · 2020-09-15T17:41:55Z

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

jywarren · 2020-09-15T17:43:20Z

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

wow this is obscure!

jywarren · 2020-09-15T17:47:14Z

Made an issue here on the NPM module - mattinsler/resig-class#2

jywarren · 2020-09-15T17:55:00Z

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... 😅 )

jywarren · 2020-09-15T17:56:59Z

OK, so i added:

  "sourceType": "script",

to .babelrc, and got:

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

jywarren · 2020-09-15T18:00:00Z

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

jywarren · 2020-09-15T18:00:57Z

Fingers 🤞 !!!

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

jywarren · 2020-09-15T18:03:38Z

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?

jywarren · 2020-09-17T17:00:41Z

Wow - the folks at mattinsler/resig-class#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... 😅

jywarren · 2020-09-17T17:52:29Z

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!

jywarren · 2020-09-17T17:55:47Z

Aha searched around for this error "Jest ReferenceError: regeneratorRuntime is not defined" - here is the issue, i think! jestjs/jest#7579 (comment)

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 · 2020-09-17T17:56:47Z

.babelrc

@@ -0,0 +1,4 @@
+{
+    ['@babel/preset-env', { targets: { node: 'current' } }],


tried this!

Shulammite-Aso · 2020-09-17T18:18:36Z

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

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"

jywarren · 2020-09-17T18:22:06Z

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 😅

jywarren · 2020-09-17T18:35:59Z

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"
  }
}]
]

jywarren · 2020-09-17T18:48:18Z

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?

jywarren · 2020-09-17T18:54:41Z

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!

jywarren · 2020-09-22T21:26:15Z

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!

cypherean · 2020-09-24T04:00:28Z

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! 🎉 🎉

Sagarpreet · 2020-09-29T14:20:08Z

I will also try this 👍

jywarren · 2020-10-14T03:26:39Z

Duh... i realized, I gotta remember that we don't need puppeteer for the Jasmine tests, and those are the ones failing right now... will try them now in gitpod.

jywarren · 2020-10-14T03:37:42Z

OK, and noting that PL is defined in the example in the browser JS console...

jywarren · 2020-10-14T03:39:50Z

OH PL is defined in the browser JS console, but it does show this error:

Uncaught TypeError: Class.extend is not a function
    at Object.182.resig-class (PublicLab.Editor.js:2422)
    at s (PublicLab.Editor.js:1)
    at PublicLab.Editor.js:1
    at Object.178../PublicLab.Errors.js (PublicLab.Editor.js:2361)
    at s (PublicLab.Editor.js:1)
    at e (PublicLab.Editor.js:1)
    at PublicLab.Editor.js:1

jywarren · 2020-10-14T03:40:45Z

On this line:

PublicLab.Editor/src/adapters/PublicLab.Formatter.js

Line 8 in 0daf8cb

module.exports = PublicLab.Formatter = Class.extend({

jywarren · 2020-10-14T03:42:35Z

I think usage of resig-class changed -- it's now const { Class } = require('resig-class');

jywarren · 2020-10-14T04:02:11Z

OK - well, i got Jasmine working in the browser, at /tests.html and it passes -- interesting, and useful - it'll even run in the github pages demo:

But, it's still failing when run from the terminal. But, progress, as I got Class to define properly in resig-class!

jywarren · 2020-10-14T04:05:34Z

All jasmine tests now added to the browser runner:

That last one is OK because it's looking for a local file but is getting a browser file path.

jywarren · 2020-10-14T04:09:30Z

I wonder if, in Jasmine, the global definition on these lines is not getting created:

PublicLab.Editor/src/PublicLab.Editor.js

Line 3 in 2741425

PL = PublicLab = {};

Could we pre-declare them, and then the global-ness wouldn't matter?

…ntomjs

jywarren · 2020-10-14T04:32:44Z

Trying to switch to Jasmine v2, which uses Headless Chrome, not Phantomjs, so it'll run in the same system as Jest.

jywarren · 2020-10-14T04:39:44Z

Hmm. Now, it looks like we got past the initial PL undefined error, but the constructor is not properly creating an editor object:

   - has log, key, interval, and id...X has log, key, interval, and id
     TypeError: Cannot read property 'history' of undefined (1)
>> TypeError: Cannot read property 'history' of undefined TypeError: Cannot read property 'history' of undefined
>>     at UserContext.<anonymous> (file:///home/travis/build/publiclab/PublicLab.Editor/spec/javascripts/history_spec.js:14:19)
>>     at <Jasmine>

https://travis-ci.com/github/publiclab/PublicLab.Editor/jobs/399261338#L309

jywarren · 2020-10-14T04:42:09Z

I don't know, folks, I'm out of energy for now... I wonder if we should just think of refactoring all the Jasmine tests into Jest???

jywarren · 2020-10-14T15:13:51Z

For reference, it looks like Jest and Jasmine test syntax is really similar:

// Jasmine:

it("exists, and has a textarea", function() {
  expect($('.ple-textarea')[0]).not.toBeUndefined();
  expect(editor).not.toBeUndefined();
  expect(editor.options.textarea).not.toBeUndefined();
  expect(editor.options.textarea).toBe($('.ple-textarea')[0]);
});

// Jest:

test('adds 1 + 2 to equal 3', () => {
  expect(sum(1, 2)).toBe(3);
});

https://github.com/publiclab/PublicLab.Editor/blob/main/spec/javascripts/editor_spec.js

https://github.com/facebook/jest/#getting-started

jywarren · 2020-10-14T15:20:26Z

I've refactored the button_spec.js Jasmine test into Jest. Let's see.

jywarren · 2020-10-14T17:23:23Z

:-////

 FAIL  test/ui-testing/button.test.js
  Publish button
    ✕ Publish button is enabled (2 ms)
  ● Publish button › Publish button is enabled
    ReferenceError: PL is not defined
       7 |   await page.goto('file://' + path, {waitUntil: 'domcontentloaded'});
       8 | 
    >  9 |   editor = new PL.Editor({
         |   ^
      10 |     textarea: $('.ple-textarea')[0]
      11 |   });
      12 | });
      at Object.beforeAll (test/ui-testing/button.test.js:9:3)
  ● Publish button › Publish button is enabled
    ReferenceError: $ is not defined
      15 |   test("Publish button is enabled", function() {
      16 |     // Check initially that Publish button is disabled.
    > 17 |     expect($('.ple-publish').prop('disabled')).toBe(true);
         |     ^
      18 |     // Add title.
      19 |     $('.ple-module-title input').val('A title');
      20 |     $('.ple-module-title input').keydown();
      at Object.<anonymous> (test/ui-testing/button.test.js:17:5)

sagarpreet-chadha · 2020-10-14T18:27:29Z

Maybe this issue I faced long time back is related to this: publiclab/leaflet-blurred-location-display#52

jywarren · 2020-10-14T19:35:14Z

Hi @Sagarpreet it's possible regarding your comment on phantomjs, but grunt-contrib-jasmine was refactored in v2 to use headless-chrome, instead of phantomjs, so that should no longer be an issue, i think?

jywarren · 2020-10-14T19:36:00Z

i think it could be related overall though - are we declaring strict mode, and perhaps our attempt within the editor library to define globals is not working because that's no longer allowed in ES6 or strict mode?

jywarren · 2021-01-26T23:21:40Z

Noting that we're seeing the PL not defined error on all PRs, i think! It could be unrelated! https://travis-ci.com/github/publiclab/PublicLab.Editor/jobs/399630193

jywarren · 2021-01-27T01:31:39Z

Much progress in #680 !!!

jywarren · 2021-01-27T21:40:16Z

OK! Much of this is now solved due to the grunt-contrib-jasmine 2.x upgrade in #680, but if we took out the sections of package.json plus the .babelrc, and possibly the new resig-class implementation, this could be adapted to work too; it might be worth pulling that stuff out and starting over as the complicated stuff we tried is no longer necessary!

And, the original reason was that Jasmine couldn't do ES6 which is now no longer the case, so there isn't as strong a reason for this now. But Babel is nice so if anyone wants to try, go for it!

jywarren reviewed Sep 14, 2020

View reviewed changes

sagarpreet-chadha requested changes Sep 14, 2020

View reviewed changes

jywarren reviewed Sep 15, 2020

View reviewed changes

.travis.yml

script:

- npm run build

Copy link

Member

jywarren Sep 15, 2020

Choose a reason for hiding this comment

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

i'm going ot try this as a before_script so we are sure it's done before it tries the tests.

jywarren mentioned this pull request Sep 15, 2020

doesn't work in strict mode mattinsler/resig-class#2

Closed

jywarren reviewed Sep 17, 2020

View reviewed changes

.babelrc Outdated

@@ -0,0 +1,4 @@

{

['@babel/preset-env', { targets: { node: 'current' } }],

Copy link

Member

jywarren Sep 17, 2020

Choose a reason for hiding this comment

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

tried this!

removing the puppeteer stuff

a9f13c9

working in-browser Jasmine tests at /tests.html

a71bf41

all jasmine tests in browser

88271c6

switch to grunt-contrib-jasmine v2 for headless chrome instead of pha…

bf4b5c1

…ntomjs

refactor button test into jest

bedfc0c

jywarren added 4 commits October 14, 2020 15:26

split into jobs in travis

23c62aa

fix for new jest test

3902111

custom insert jest test fix

c2c363b

more jest fixes

f730a88

jywarren mentioned this pull request Jan 27, 2021

Try running tests in GitHub Actions #680

Merged

		@@ -0,0 +1,4 @@
		{
		['@babel/preset-env', { targets: { node: 'current' } }],

Setup babel #614

Are you sure you want to change the base?

Setup babel #614

Conversation

Shulammite-Aso commented Sep 13, 2020

gitpod-io bot commented Sep 13, 2020 • edited Loading

Shulammite-Aso commented Sep 13, 2020 • edited by jywarren Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagarpreet-chadha commented Sep 14, 2020

jywarren commented Sep 15, 2020

Choose a reason for hiding this comment

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 15, 2020

jywarren commented Sep 17, 2020

jywarren commented Sep 17, 2020

jywarren commented Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Shulammite-Aso commented Sep 17, 2020

jywarren commented Sep 17, 2020

jywarren commented Sep 17, 2020

jywarren commented Sep 17, 2020

jywarren commented Sep 17, 2020

jywarren commented Sep 22, 2020

cypherean commented Sep 24, 2020

Sagarpreet commented Sep 29, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020 • edited Loading

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020 • edited Loading

jywarren commented Oct 14, 2020 • edited Loading

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

sagarpreet-chadha commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Oct 14, 2020

jywarren commented Jan 26, 2021

jywarren commented Jan 27, 2021

jywarren commented Jan 27, 2021

gitpod-io bot commented Sep 13, 2020 •

edited

Loading

Shulammite-Aso commented Sep 13, 2020 •

edited by jywarren

Loading

jywarren commented Sep 17, 2020 •

edited

Loading

jywarren commented Oct 14, 2020 •

edited

Loading

jywarren commented Oct 14, 2020 •

edited

Loading

jywarren commented Oct 14, 2020 •

edited

Loading