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(1684): update ember to 3.16 #576

Merged
merged 16 commits into from
Aug 6, 2020

Conversation

wahapo
Copy link
Contributor

@wahapo wahapo commented Jul 31, 2020

Context

Fix this issue screwdriver-cd/screwdriver#1684.

We can reproduce it by entering the following yaml into validator.

jobs:
  main:
    image: node:10
    steps:
      - ls
    annotations:
      sd.cd: 1 

Objective

This PR will fix it, as shown in the screenshot below.

It's because of this bug.
emberjs/ember.js#17529

We can fix it by updating to at least v3.13.0. I would suggest giving it up to a version of LTS.

References

screwdriver-cd/screwdriver#1684
https://emberjs.com/releases/lts
adopted-ember-addons/ember-light-table#701
https://deprecations.emberjs.com/v3.x/#toc_jquery-apis

License

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

jithine
jithine previously approved these changes Jul 31, 2020
@jithine
Copy link
Member

jithine commented Jul 31, 2020

Looks like few tests failed.

cc @adong

package.json Outdated
@@ -69,7 +69,7 @@
"ember-ace": "^2.0.1",
"ember-ajax": "^5.0.0",
"ember-bootstrap": "^2.8.1",
"ember-cli": "~3.10.1",
"ember-cli": "^3.20.0",
Copy link
Member

@jithine jithine Jul 31, 2020

Choose a reason for hiding this comment

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

if we are planning to stick to LTS shouldn't it be 3.16 ? https://github.com/emberjs/ember.js/blob/v3.16.6/CHANGELOG.md

@jithine jithine dismissed their stale review July 31, 2020 15:17

Missed one aspect

@adong
Copy link
Member

adong commented Jul 31, 2020

Nice. I like 3.16, which is the latest LTS https://emberjs.com/releases/lts

cd screwdriver-cd/ui

# optional and important, some dependencies may not in enterprise npm registry yet
# npm config set registry https://registry.npmjs.org/

npm install -g ember-cli-update
ember-cli-update --to 3.16.0

Fix eslint styles:

node ./node_modules/eslint/bin/eslint.js --fix .

Thank you for taking a stab at it 👍

Refer:

  1. https://github.com/ember-cli/ember-cli-update has more details.

@wahapo wahapo changed the title fix(1684): update ember to 3.16.6 fix(1684): update ember to 3.16 Aug 3, 2020
.travis.yml Outdated Show resolved Hide resolved
@wahapo
Copy link
Contributor Author

wahapo commented Aug 4, 2020

I feel like this stub is not being replaced correctly. 🤔

@adong
Copy link
Member

adong commented Aug 4, 2020

I feel like this stub is not being replaced correctly. 🤔

ember-sinon-quint has a newer version for ember 3.16: https://github.com/elwayman02/ember-sinon-qunit/releases/tag/v5.0.0

To install the newer version, we need to upgrade package.json:

    "ember-sinon": "^5.0.0",
    "ember-sinon-qunit": "^5.0.0",

Also we need to have the following for tests/test-helper.js:

import { setApplication } from '@ember/test-helpers'; 
import { start } from 'ember-qunit'; 
import Application from '../app'; 
import config from '../config/environment'; 
import setupSinon from 'ember-sinon-qunit';
 
setApplication(Application.create(config.APP)); 
 
setupSinon();
 
start(); 

@adong
Copy link
Member

adong commented Aug 4, 2020

I also noticed that

  result: computed('value', {
    async get() {

in app/components/pipeline-list-coverage-cell/component.js needs to be changed to

  result: computed('value', {
    get: async function() {

I tested on babel

image

It seems the first way of written was never supported. 🤷‍♂️

Refer:

  1. async getters discussed? tc39/proposal-async-await#15
  2. 0011-improved-cp-syntax.md

@adong
Copy link
Member

adong commented Aug 5, 2020

Looks like ember-sinon-quit has deprecated https://github.com/elwayman02/ember-sinon-qunit#deprecated-features since https://github.com/elwayman02/ember-sinon-qunit/tree/aab1b20c198f104bd02e6fd4c1b610e5c7f88f30

image

Do the following in tests/unit/application/route-test.js will fix test

# These 2 are new
import { later } from '@ember/runloop';
import sinon from 'sinon';

  test('it should clear store and reload page on session change', function(assert) {
    const route = this.owner.lookup('route:application');
    const session = this.owner.lookup('service:session');
    const reloadStub = sinon.stub(route, 'reloadPage');

    session.set('data.sessionChanged', true);

    later(() => {
      assert.ok(reloadStub.calledOnce, 'reloadPage was not called');
    });
  });

  test('it should not clear store and reload page if no session change', function(assert) {
    const route = this.owner.lookup('route:application');
    const session = this.owner.lookup('service:session');
    const reloadStub = sinon.stub(route, 'reloadPage');

    session.set('data.sessionChanged', false);

    later(() => {
      assert.notOk(reloadStub.calledOnce, 'reloadPage was called');
    });
  });

Wrap with run.later will make sure it is executed until other actions were called, in this case, it is exactly what we need.

@wahapo
Copy link
Contributor Author

wahapo commented Aug 5, 2020

Cool! That's exactly what I was looking for right now.

package.json Outdated
"ember-export-application-global": "^2.0.0",
"ember-data": "~3.16.0",
"ember-export-application-global": "^2.0.1",
"ember-fetch": "^7.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Please remove ember-fetch, since we already have ember-ajax

@adong
Copy link
Member

adong commented Aug 5, 2020

@wahapo

2 questions,

  1. I found eslint . --fix is not fixing anything for me. It's very frustrating, and I'm curious how are you approaching the linter errors?

  2. Do you happen to know which rule made an extra line between let variable declaration?

Thanks

@wahapo
Copy link
Contributor Author

wahapo commented Aug 5, 2020

@adong

  1. I just run npm run "lint:fix" command.
  2. Eslint said error Expected blank line after variable declarations newline-after-var. It may have something to do with the fact that the version of eslint is v5.16.0.
$ ./node_modules/eslint/bin/eslint.js -v
v5.16.0

@adong
Copy link
Member

adong commented Aug 5, 2020

Very strange. If I run the following command to run test locally.

CI=true npm test

Output:

1..1064
# tests 1064
# pass  1064
# skip  0
# fail  0

# ok

All tests passed. Not sure whats going on.

@wahapo
Copy link
Contributor Author

wahapo commented Aug 6, 2020

All the tests are now passed. I'd like you to actually try running the UI and check if there are any problems. I'm going to check it now too.

Copy link
Member

@adong adong left a comment

Choose a reason for hiding this comment

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

I tested locally, and have not found any issues. Approved

ibu1224
ibu1224 previously approved these changes Aug 6, 2020
Copy link
Member

@adong adong left a comment

Choose a reason for hiding this comment

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

For config/environment.js

      'script-src': ["'unsafe-eval'"],

I removed this locally, and not see the effect of removing. Do we need it?

let left = this.left ? event.layerX - 20 : event.layerX - el.offsetWidth / 2;

el.style.top = top;
el.style.left = left;
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the old logics.

  didUpdateAttrs() {
    this._super(...arguments);

    const event = get(this, 'tooltipData.mouseevent');
    const el = this.$();

    // setting tooltip position
    if (el && event) {
      let top = event.layerY + get(this, 'tooltipData.sizes.ICON_SIZE');
      let left = this.left ? event.layerX - 20 : event.layerX - el.outerWidth() / 2;

      el.css({
        top,
        left
      });
    }
  }

This resulted tooltip not position correctly, after changed back to old code, it renders correctly.
image

Copy link
Member

@adong adong Aug 6, 2020

Choose a reason for hiding this comment

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

I looked again, I think its this issue. el.css() add px for us, while handle css via style, we have to add px ourselves.

In that case, we can either use the old el.css behavior, or update to

      el.style.top = `${top}px`;
      el.style.left = `${left}px`;

Ref: https://stackoverflow.com/questions/10351291/unable-to-set-div-style-left-and-div-style-top-css-properties-via-javascript

href: this.logService.buildLogBlobUrl(buildId, stepName)
})[0]
.click();
const el = this.element.querySelectorAll('#downloadLink');
Copy link
Member

Choose a reason for hiding this comment

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

Minor optimization:

Because of the #, there ought to be only one element with that id, so I think this is the same as

const el = this.element.querySelector('#downloadLink');
el.setAttribute('download', `${buildId}-${stepName}.log`);
el.setAttribute('href', this.logService.buildLogBlobUrl(buildId, stepName));
el.click();

While querySelectorAll('#downloadLink'); always returns an array with single element.

"application-template-wrapper": false,
"default-async-observers": true,
"jquery-integration": true,
"template-only-glimmer-components": true
Copy link
Member

Choose a reason for hiding this comment

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

"template-only-glimmer-components": true

This feature breaks 404 page,

image

and changes html output to

image

Solution:

It is recommended that you enable this feature. Existing applications adopting this optional feature should add a .js file for any existing template-only components containing a basic Ember component class. This will maintain backwards compatibility for existing templates while new template-only components gain the advantages of this feature. (https://guides.emberjs.com/release/configuring-ember/optional-features/#toc_template-only-glimmer-components)

create a file app/components/404-display/component.js

import Component from '@ember/component';

export default Component.extend({});

Copy link
Member

@adong adong Aug 6, 2020

Choose a reason for hiding this comment

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

One contributor ember-component-css from said this is an issue from "ember-component-css": "^0.6.9", there's an alpha version to resolve it, but we are not going to incorporate now. If you are interested in, please follow-up here: webark/ember-component-css#342 (comment)

Copy link
Contributor Author

@wahapo wahapo Aug 6, 2020

Choose a reason for hiding this comment

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

Ok, I think that's fine for now, too. Thanks!

@wahapo
Copy link
Contributor Author

wahapo commented Aug 6, 2020

I don't remember why I added 'script-src': ["'unsafe-eval'"], but now that I've tried it, it doesn't seem to be a problem, so I'll remove it. Thanks.

@@ -5,7 +5,9 @@ export default Component.extend({
actions: {
nameClick() {
this.toggleProperty('isOpen');
this.$('#validator-ace-editor').toggle('hidden');
this.element
Copy link
Member

Choose a reason for hiding this comment

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

Similar here, this.querySelector is preferred when querying for #id

@@ -178,3 +178,5 @@ body {
}

@import 'ember-power-select';

@import 'ember-bootstrap/bootstrap';
Copy link
Member

Choose a reason for hiding this comment

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

@import 'ember-bootstrap/bootstrap' is duplicated in L67 app/styles/app.scss

@wahapo
Copy link
Contributor Author

wahapo commented Aug 6, 2020

There may be something wrong with job-list-view. Select job-list-view and refresh it, and select job-graph-view. After doing it a few times, the list-view remained visible.

@adong
Copy link
Member

adong commented Aug 6, 2020

There may be something wrong with job-list-view. Select job-list-view and refresh it, and select job-graph-view. After doing it a few times, the list-view remained visible.

  1. I don't see this PR has logics changes to the list-view & graph-view
  2. I'm not able to reproduce it locally 😅

I was thinking maybe we merge this PR, and see. We can always do a bug-fix, if it's reproducible :)

@wahapo
Copy link
Contributor Author

wahapo commented Aug 6, 2020

Maybe my locally is just weird. I agree with that policy.

@adong adong merged commit 2f1531d into screwdriver-cd:master Aug 6, 2020
@wahapo wahapo deleted the issue-1684 branch August 6, 2020 06:31
@adong
Copy link
Member

adong commented Aug 6, 2020

Merged as feat(1684)

@wahapo Thank you for doing the ember migration, great job! Congrats. Now we don't have to worry about ember migration for this year. Truly amazing.
Next, we are going to pull ember-denali

ember install @denali-design/ember@1.0.0-alpha.8

@wahapo
Copy link
Contributor Author

wahapo commented Aug 6, 2020

@adong I appreciate the many comments and advice. Thank you so much! I hope this commit works well🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants