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

chore: update dependency #128

Merged
merged 2 commits into from Jan 20, 2020
Merged

chore: update dependency #128

merged 2 commits into from Jan 20, 2020

Conversation

@jannyHou
Copy link
Contributor

jannyHou commented Jan 13, 2020

Description

Update according to snyk report

Related issues

  • connect to <link_to_referenced_issue>

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide
@jannyHou jannyHou requested a review from raymondfeng as a code owner Jan 13, 2020
@jannyHou jannyHou force-pushed the dep-update branch from 73ded7b to 8fa9390 Jan 14, 2020
src/doc.ts Outdated
import * as marked from 'marked';
import * as path from 'path';
import * as string from 'underscore.string';
import {Annotation} from './annotation';
import {Docs} from './docs';
import {Comment} from './dox';
import {Options, Section} from './ts-helper';
const hljs = require('highlight.js');

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 14, 2020

Member

Why is it needed?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jan 15, 2020

Author Contributor

my vscode was complaining, maybe I used a wrong TypeScript version for my workspace. Now it's gone. Thank you Raymond for the update!

@@ -66,7 +66,8 @@ describe('TypeScript Parser Test', function() {
}
);

it('should report errors if es2016 apis are used with es2015 tsconfig', function() {
// TypeScript 3.7.x no longer reports it as an error
it.skip('should report errors if es2016 apis are used with es2015 tsconfig', function() {

This comment has been minimized.

Copy link
@dhmlau

dhmlau Jan 15, 2020

Member

should we simply remove this test case then?

This comment has been minimized.

Copy link
@bajtos

bajtos Jan 16, 2020

Member

Isn't this a matter of typescript configuration? I think it should be possible to configure TypeScript to reject ES2016 (and newer) APIs.

See https://www.typescriptlang.org/docs/handbook/compiler-options.html, the part --lib.

List of library files to be included in the compilation.

Possible values are:
► ES5
► ES6
► ES2015
► ES7
► ES2016
► ES2017
► ES2018
► ESNext
► DOM
► DOM.Iterable
► WebWorker
► ScriptHost
► ES2015.Core
► ES2015.Collection
► ES2015.Generator
► ES2015.Iterable
► ES2015.Promise
► ES2015.Proxy
► ES2015.Reflect
► ES2015.Symbol
► ES2015.Symbol.WellKnown
► ES2016.Array.Include
► ES2017.object
► ES2017.Intl
► ES2017.SharedMemory
► ES2017.String
► ES2017.TypedArrays
► ES2018.Intl
► ES2018.Promise
► ES2018.RegExp
► ESNext.AsyncIterable
► ESNext.Array
► ESNext.Intl
► ESNext.Symbol

Note: If --lib is not specified a default list of libraries are injected. The default libraries injected are:
► For --target ES5: DOM,ES5,ScriptHost
► For --target ES6: DOM,ES6,DOM.Iterable,ScriptHost

I think the more important question to ask is why are we rejecting ES2016 syntax? Is it because it was difficult to support it in the past? If TypeScript no longer throws now, then maybe we should change the test to verify that ES2016 syntax is allowed after the dependencies are upgraded?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 16, 2020

Member

We have https://github.com/strongloop/strong-docs/blob/master/test/tsconfig.json#L9 to configure the lib to es2015. The test was there to capture the fact that TypeScript rejects the usage of non es2015 apis.

We can change the test to accept the fact that latest TypeScript does not report an error for such case. The behavior won't impact how strong docs are generated anyway.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jan 16, 2020

Author Contributor

then maybe we should change the test to verify that ES2016 syntax is allowed after the dependencies are upgraded?

After following the discussion here I agree with rewrite the test to reflect the fact.

Copy link
Member

bajtos left a comment

Are there any notable breaking changes to be aware of? Did you check the rendered HTML output to verify it looks (mostly) the same as before the upgrade? (See e.g. http://apidocs.loopback.io/loopback/.)

@@ -66,7 +66,8 @@ describe('TypeScript Parser Test', function() {
}
);

it('should report errors if es2016 apis are used with es2015 tsconfig', function() {
// TypeScript 3.7.x no longer reports it as an error
it.skip('should report errors if es2016 apis are used with es2015 tsconfig', function() {

This comment has been minimized.

Copy link
@bajtos

bajtos Jan 16, 2020

Member

Isn't this a matter of typescript configuration? I think it should be possible to configure TypeScript to reject ES2016 (and newer) APIs.

See https://www.typescriptlang.org/docs/handbook/compiler-options.html, the part --lib.

List of library files to be included in the compilation.

Possible values are:
► ES5
► ES6
► ES2015
► ES7
► ES2016
► ES2017
► ES2018
► ESNext
► DOM
► DOM.Iterable
► WebWorker
► ScriptHost
► ES2015.Core
► ES2015.Collection
► ES2015.Generator
► ES2015.Iterable
► ES2015.Promise
► ES2015.Proxy
► ES2015.Reflect
► ES2015.Symbol
► ES2015.Symbol.WellKnown
► ES2016.Array.Include
► ES2017.object
► ES2017.Intl
► ES2017.SharedMemory
► ES2017.String
► ES2017.TypedArrays
► ES2018.Intl
► ES2018.Promise
► ES2018.RegExp
► ESNext.AsyncIterable
► ESNext.Array
► ESNext.Intl
► ESNext.Symbol

Note: If --lib is not specified a default list of libraries are injected. The default libraries injected are:
► For --target ES5: DOM,ES5,ScriptHost
► For --target ES6: DOM,ES6,DOM.Iterable,ScriptHost

I think the more important question to ask is why are we rejecting ES2016 syntax? Is it because it was difficult to support it in the past? If TypeScript no longer throws now, then maybe we should change the test to verify that ES2016 syntax is allowed after the dependencies are upgraded?

@@ -1,6 +1,6 @@
<section class="code-doc ">
<a name="<%-anchorId%>"></a>
<h3 class="code-ref"><%-kindString%>: <%-name%></h3>
<% include comments %>
<%- include('comments'); %>

This comment has been minimized.

Copy link
@bajtos

bajtos Jan 16, 2020

Member

Are you sure these include statements should be terminated by a semicolon? I would expect something like this:

Suggested change
<%- include('comments'); %>
<%- include('comments') %>

I am not very familiar with EJS, so it's very likely I am wrong.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 16, 2020

Member

include(...) is just a JS function call. The trailing ; is optional but I prefer to have it for consistency.

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Jan 17, 2020

@bajtos

Are there any notable breaking changes to be aware of? Did you check the rendered HTML output to verify it looks (mostly) the same as before the upgrade?

Good reminder, I compared the current code and my branch on local and clicked links, don't see any difference:

Master branch:
Screen Shot 2020-01-17 at 12 44 09 PM

My branch:
Screen Shot 2020-01-17 at 12 52 54 PM

jannyHou and others added 2 commits Jan 13, 2020
@jannyHou jannyHou force-pushed the dep-update branch from 18775fe to ae7a42e Jan 17, 2020
@jannyHou jannyHou merged commit 5eb956b into master Jan 20, 2020
10 checks passed
10 checks passed
Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] downstream: apidocs.strongloop.com@master Success! (ae7a42e)
Details
[cis-jenkins] x64 && linux && nvm,10 Success! (ae7a42e)
Details
[cis-jenkins] x64 && linux && nvm,12 Success! (ae7a42e)
Details
[cis-jenkins] x64 && linux && nvm,8 Success! (ae7a42e)
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@delete-merged-branch delete-merged-branch bot deleted the dep-update branch Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.