-
Notifications
You must be signed in to change notification settings - Fork 170
Windows Platform Release Preparation (Cordova 9) #319
Conversation
@@ -50,7 +50,9 @@ module.exports = function patch (platform) { | |||
// 1. Find start page path in appxmanifest | |||
var AppxManifest = require('./lib/AppxManifest'); | |||
var appxmanifest = AppxManifest.get(path.join(__dirname, '..', appxmanifestMap[platform])); | |||
var startPage = url.parse(appxmanifest.getApplication().getStartPage()); | |||
|
|||
// @todo Use 'url.URL' constructor instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESLint Notice
53:21 error 'url.parse' was deprecated since v11.0.0. Use 'url.URL' constructor instead node/no-deprecated-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would favor fixing this one before making the new release, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ESLint notice is for Node 11+, when it becomes deprecated and which we do not support yet.
Per documentations, changes would be as follows:
var startPage = new URL(appxmanifest.getApplication().getStartPage());
or
var startPage = new url.URL(appxmanifest.getApplication().getStartPage());
appxmanifest.getApplication().getStartPage()
pulls the start page from package.*.appxmanifest
, which is www/index.html
.
Here is the output of the current usage url.parse("www/index.html")
:
Url {
protocol: null,
slashes: null,
auth: null,
host: null,
port: null,
hostname: null,
hash: null,
search: null,
query: null,
pathname: 'www/index.html',
path: 'www/index.html',
href: 'www/index.html' }
Now, let's take a look at what new URL("www/index.html")
and new url.URL("www/index.html")
prints out with our supported Node versions.
Node 6
with new URL("www/index.html")
:
ReferenceError: URL is not defined
with new url.URL("www/index.html")
:
TypeError: Invalid URL: www/index.html
Node 8
new URL("www/index.html")
:
ReferenceError: URL is not defined
new url.URL("www/index.html")
:
TypeError [ERR_INVALID_URL]: Invalid URL: www/index.html
at onParseError (internal/url.js:219:17)
at parse (internal/url.js:228:3)
at new URL (internal/url.js:311:5)
Node 10
new URL("www/index.html")
and
new url.URL("www/index.html")
:
{ TypeError [ERR_INVALID_URL]: Invalid URL: www/index.html
at onParseError (internal/url.js:240:17)
at parse (internal/url.js:249:3)
at new URL (internal/url.js:324:5) input: 'www/index.html' }
Since we do not support Node 11 currently, I hope that this can be postponed till after the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 from me
Platforms affected
windows
What does this PR do?
This PR contains final preparations for the Cordova 9 release goals.
See Cordova 9 Release Plan.
cordova-common@^3.1.0
eslint@^5.12.0
eslint-config-semistandard@^13.0.0
eslint-config-standard@^12.0.0
eslint-plugin-import@^2.14.0
eslint-plugin-node@^8.0.1
eslint-plugin-promise@^4.0.1
eslint-plugin-standard@^4.0.0
[email protected]
rewire@^4.0.1
nopt@^4.0.1
nyc@^13.1.0
What testing has been done on this change?
npm run eslint
npm run cover
(unit testing)