Skip to content

Instantly share code, notes, and snippets.

@walkermatt

walkermatt/_talk-builds-tests-bugs-semver.md

Last active January 13, 2021 12:36
Show Gist options
  • Save walkermatt/f3f8468b263f05d3992ba3fdb4c611c3 to your computer and use it in GitHub Desktop.
Save walkermatt/f3f8468b263f05d3992ba3fdb4c611c3 to your computer and use it in GitHub Desktop.
Download ZIP
Talk on small contributions to open source software, testing, automated builds etc.
Raw
_talk-builds-tests-bugs-semver.md

The day the build broke 😔

The tale of a broken build and breaking changes...

And so the story begins...

  • ol-ishare is built automatically each time a change is committed to the repository
    • Formatting of source code is checked (linting)
    • Unit tests are ran
    • Library and apps such as LiteMap, Spotlight etc. are built
    • Examples and apps are deployed to S3

Behold a broken build!

Test Suites: 2 failed, 9 passed, 11 total
Tests:       9 failed, 116 passed, 125 total
Snapshots:   19 passed, 19 total
Time:        28.249 s

Digging in

The failed tests were testing functionality that hasn't changed recently in ol-ishare so must be due to something else changing.

● getFeatureByValue › Returns features when request is successful and features are found

TypeError: Cannot read property 'PropertyIsEqualTo' of undefined
  449 |  */
  450 | function getFeatureByValue(owsUrl, layerName, column, value, callback) {
> 451 |   var filter = writeFilter(equalTo(column, value));
      |                ^
  452 |   getFeature(owsUrl, layerName, filter, null, callback);
  453 | }
  454 | 
  at serialize (node_modules/ol/src/xml.js:491:9)
  at pushSerializeAndPop (node_modules/ol/src/xml.js:535:3)
  at writeFilterCondition (node_modules/ol/src/format/WFS.js:1067:3)
  at writeFilter (node_modules/ol/src/format/WFS.js:1339:3)
  at Object.getFeatureByValue (src/esm/info.js:451:16)
  at Object.<anonymous> (test/info.test.js:327:10)

More digging

  • The issue was eventually narrowed down to a change in ol/format/WFS~writeFilter function
  • The build installed [email protected] while we had been using the previous release [email protected]
  • [email protected] had introduced support for WFS 2.0
    • writeFilter had been updated to include a WFS version argument
      • due to how an OGC Filter is written to XML changed in WFS 2.0

Behold a fix!

The fix was very straightforward, specify the WFS version we're using:

diff --git a/src/esm/info.js b/src/esm/info.js
index 908f89d..34e7260 100644
--- a/src/esm/info.js
+++ b/src/esm/info.js
@@ -448,7 +448,7 @@ function getFeature(owsUrl, layerName, filter, params, callback) {
  * @param {module:ol-ishare/info~getFeatureCallback} callback Function to be called with features...
  */
 function getFeatureByValue(owsUrl, layerName, column, value, callback) {
-  var filter = writeFilter(equalTo(column, value));
+  var filter = writeFilter(equalTo(column, value), '1.0.0');
   getFeature(owsUrl, layerName, filter, null, callback);
 }

We could of walked away at this point

  • We've identified the issue and fixed it ✔️
  • But it's still an issue for everyone else using OpenLayers ❌

Leave it better than you found it 🌳

  • Adding a mandatory argument to an existing function is a breaking change
    • It's a tiny change but significant
  • My original intention was a small Pull Request to make the version argument optional in OpenLayers to avoid a breaking change
    • Add a test for the change
    • Make the change

There's more... 🐃

  • There was no test for calling writeFilter with the version set to 2.0.0
    • So I added a test, which failed 💥
  • Luckily it was an easy fix

Pull Request with tests and changes: openlayers/openlayers#11890

OpenLayers v6.5.0 should of been v7.0.0

  • When a breaking change is introduced to a public API the Major version should be increase

Semantic Versioning

Given a version number MAJOR.MINOR.PATCH, increment the:

  • MAJOR version when you make incompatible API changes,
  • MINOR version when you add functionality in a backwards compatible manner, and
  • PATCH version when you make backwards compatible bug fixes.
  • Optional PRE-RELEASE version for beta, alpha, release candidate etc.
                6.5.0-beta
                | | |   |
MAJOR version --+ | |   |
MINOR version ----+ |   |
PATCH version ------+   |
PRE-RELEASE version ----+

Full spec: https://semver.org/

Semantic Versioning cont.

  • Important for APIs
  • Very well established convention
  • But not everyone sticks to it...
  • Traditionally people treat MAJOR version changes to be a release with significant new functionality

Specifying versions in package.json

Dependencies are defined in package.json

"dependencies": {
  "ol": "~6.5.0",
  "ol-layerswitcher": "^3.8.1",
  "ol-popup": ">=4.0.0",
  ...
},

There are number of ways to specify an acceptable version:

  • Exact version 6.5.0
  • Same major range ^6.5.0 would allow 6.5.1, 6.6.0 but not 7.0.0
  • Same minor range ~6.5.0 would allow 6.5.1, 6.5.2 but not 6.6.0
  • A range of stable versions >, <, =, >= or <= for comparisons, or - to specify an inclusive range

Further details at: https://semver.npmjs.com/

Raw
build.sh
npx @marp-team/marp-cli --template bare -o talk-builds-tests-bugs-semver.html _talk-builds-tests-bugs-semver.md
Raw
talk-builds-tests-bugs-semver.html
<!DOCTYPE html><html lang="en-GB"><head><meta charset="UTF-8"><meta name="viewport" content="width=device-width,height=device-height,initial-scale=1.0"><meta name="apple-mobile-web-app-capable" content="yes"><meta http-equiv="X-UA-Compatible" content="ie=edge"><meta property="og:type" content="website"><meta name="twitter:card" content="summary"><style media="screen">body,html{background:#000;height:100%;margin:0;overflow:auto;-ms-scroll-snap-type:y mandatory;scroll-snap-type:y mandatory;-ms-scroll-snap-type:mandatory;scroll-snap-type:mandatory;-ms-scroll-snap-points-y:repeat(100%);scroll-snap-points-y:repeat(100%)}body>svg{display:block;height:100%;scroll-snap-align:center center;width:100%}</style><style>svg>foreignObject>section{width:1280px;height:720px;box-sizing:border-box;overflow:hidden;position:relative;scroll-snap-align:center center}svg>foreignObject>section:after{bottom:0;content:attr(data-marpit-pagination);padding:inherit;pointer-events:none;position:absolute;right:0}svg>foreignObject>section:not([data-marpit-pagination]):after{display:none}/* Normalization */svg>foreignObject>section h1{font-size:2em;margin:0.67em 0}svg>foreignObject>section video::-webkit-media-controls{will-change:transform}@page{size:1280px 720px;margin:0}@media print{body,html{background-color:#fff;margin:0;page-break-inside:avoid;break-inside:avoid-page}svg>foreignObject>section{page-break-before:always;break-before:page}svg>foreignObject>section,svg>foreignObject>section *{-webkit-print-color-adjust:exact!important;animation-delay:0s!important;animation-duration:0s!important;color-adjust:exact!important;transition:none!important}svg[data-marpit-svg]{display:block;height:100vh;width:100vw}}svg>foreignObject>section svg[data-marp-fitting=svg]{display:block;height:auto;width:100%}@supports (-ms-ime-align:auto){svg>foreignObject>section svg[data-marp-fitting=svg]{position:static}}svg>foreignObject>section svg[data-marp-fitting=svg].__reflow__{content:""}@supports (-ms-ime-align:auto){svg>foreignObject>section svg[data-marp-fitting=svg].__reflow__{position:relative}}svg>foreignObject>section [data-marp-fitting-svg-content]{display:table;white-space:nowrap}svg>foreignObject>section [data-marp-fitting-svg-content-wrap]{white-space:pre}svg>foreignObject>section img[data-marp-twemoji]{background:transparent;height:1em;margin:0 .05em 0 .1em;vertical-align:-.1em;width:1em}
/*!
* Marp default theme.
*
* @theme default
* @author Yuki Hattori
*
* @auto-scaling true
* @size 4:3 960px 720px
*/svg>foreignObject>section .octicon{display:inline-block;fill:currentColor;vertical-align:text-bottom}svg>foreignObject>section .anchor{float:left;line-height:1;margin-left:-20px;padding-right:4px}svg>foreignObject>section .anchor:focus{outline:none}svg>foreignObject>section h1 .octicon-link,svg>foreignObject>section h2 .octicon-link,svg>foreignObject>section h3 .octicon-link,svg>foreignObject>section h4 .octicon-link,svg>foreignObject>section h5 .octicon-link,svg>foreignObject>section h6 .octicon-link{color:#1b1f23;vertical-align:middle;visibility:hidden}svg>foreignObject>section h1:hover .anchor,svg>foreignObject>section h2:hover .anchor,svg>foreignObject>section h3:hover .anchor,svg>foreignObject>section h4:hover .anchor,svg>foreignObject>section h5:hover .anchor,svg>foreignObject>section h6:hover .anchor{text-decoration:none}svg>foreignObject>section h1:hover .anchor .octicon-link,svg>foreignObject>section h2:hover .anchor .octicon-link,svg>foreignObject>section h3:hover .anchor .octicon-link,svg>foreignObject>section h4:hover .anchor .octicon-link,svg>foreignObject>section h5:hover .anchor .octicon-link,svg>foreignObject>section h6:hover .anchor .octicon-link{visibility:visible}svg>foreignObject>section h1:hover .anchor .octicon-link:before,svg>foreignObject>section h2:hover .anchor .octicon-link:before,svg>foreignObject>section h3:hover .anchor .octicon-link:before,svg>foreignObject>section h4:hover .anchor .octicon-link:before,svg>foreignObject>section h5:hover .anchor .octicon-link:before,svg>foreignObject>section h6:hover .anchor .octicon-link:before{width:16px;height:16px;content:" ";display:inline-block;background-image:url("data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http://www.w3.org/2000/svg' width='16' height='16' aria-hidden='true'%3E%3Cpath fill-rule='evenodd' d='M4 9h1v1H4c-1.5 0-3-1.69-3-3.5S2.55 3 4 3h4c1.45 0 3 1.69 3 3.5 0 1.41-.91 2.72-2 3.25V8.59c.58-.45 1-1.27 1-2.09C10 5.22 8.98 4 8 4H4c-.98 0-2 1.22-2 2.5S3 9 4 9zm9-3h-1v1h1c1 0 2 1.22 2 2.5S13.98 12 13 12H9c-.98 0-2-1.22-2-2.5 0-.83.42-1.64 1-2.09V6.25c-1.09.53-2 1.84-2 3.25C6 11.31 7.55 13 9 13h4c1.45 0 3-1.69 3-3.5S14.5 6 13 6z'/%3E%3C/svg%3E")}svg>foreignObject>section{-ms-text-size-adjust:100%;-webkit-text-size-adjust:100%;color:#24292e;font-family:-apple-system,BlinkMacSystemFont,Segoe UI,Helvetica,Arial,sans-serif,Apple Color Emoji,Segoe UI Emoji;font-size:16px;line-height:1.5;word-wrap:break-word}svg>foreignObject>section{--marpit-root-font-size:16px}svg>foreignObject>section details{display:block}svg>foreignObject>section summary{display:list-item}svg>foreignObject>section a{background-color:initial}svg>foreignObject>section a:active,svg>foreignObject>section a:hover{outline-width:0}svg>foreignObject>section strong{font-weight:inherit;font-weight:bolder}svg>foreignObject>section h1{margin:.67em 0}svg>foreignObject>section img{border-style:none}svg>foreignObject>section code,svg>foreignObject>section kbd,svg>foreignObject>section pre{font-family:monospace,monospace;font-size:1em}svg>foreignObject>section hr{box-sizing:initial;overflow:visible}svg>foreignObject>section input{font:inherit;margin:0;overflow:visible}svg>foreignObject>section [type=checkbox]{padding:0}svg>foreignObject>section *,svg>foreignObject>section [type=checkbox]{box-sizing:border-box}svg>foreignObject>section input{font-family:inherit;font-size:inherit;line-height:inherit}svg>foreignObject>section a{color:#0366d6;text-decoration:none}svg>foreignObject>section a:hover{text-decoration:underline}svg>foreignObject>section strong{font-weight:600}svg>foreignObject>section hr{height:0;margin:15px 0;overflow:hidden;background:transparent;border-bottom:1px solid #dfe2e5}svg>foreignObject>section hr:after,svg>foreignObject>section hr:before{display:table;content:""}svg>foreignObject>section hr:after{clear:both}svg>foreignObject>section table{border-spacing:0;border-collapse:collapse}svg>foreignObject>section td,svg>foreignObject>section th{padding:0}svg>foreignObject>section details summary{cursor:pointer}svg>foreignObject>section h1,svg>foreignObject>section h2,svg>foreignObject>section h3,svg>foreignObject>section h4,svg>foreignObject>section h5,svg>foreignObject>section h6{margin-top:0;margin-bottom:0}svg>foreignObject>section h1{font-size:32px}svg>foreignObject>section h1,svg>foreignObject>section h2{font-weight:600}svg>foreignObject>section h2{font-size:24px}svg>foreignObject>section h3{font-size:20px}svg>foreignObject>section h3,svg>foreignObject>section h4{font-weight:600}svg>foreignObject>section h4{font-size:16px}svg>foreignObject>section h5{font-size:14px}svg>foreignObject>section h5,svg>foreignObject>section h6{font-weight:600}svg>foreignObject>section h6{font-size:12px}svg>foreignObject>section p{margin-top:0;margin-bottom:10px}svg>foreignObject>section blockquote{margin:0}svg>foreignObject>section ol,svg>foreignObject>section ul{padding-left:0;margin-top:0;margin-bottom:0}svg>foreignObject>section ol ol,svg>foreignObject>section ul ol{list-style-type:lower-roman}svg>foreignObject>section ol ol ol,svg>foreignObject>section ol ul ol,svg>foreignObject>section ul ol ol,svg>foreignObject>section ul ul ol{list-style-type:lower-alpha}svg>foreignObject>section dd{margin-left:0}svg>foreignObject>section code,svg>foreignObject>section pre{font-family:SFMono-Regular,Consolas,Liberation Mono,Menlo,monospace;font-size:12px}svg>foreignObject>section pre{margin-top:0;margin-bottom:0}svg>foreignObject>section input::-webkit-inner-spin-button,svg>foreignObject>section input::-webkit-outer-spin-button{margin:0;-webkit-appearance:none;appearance:none}svg>foreignObject>section :checked+.radio-label{position:relative;z-index:1;border-color:#0366d6}svg>foreignObject>section .border{border:1px solid #e1e4e8!important}svg>foreignObject>section .border-0{border:0!important}svg>foreignObject>section .border-bottom{border-bottom:1px solid #e1e4e8!important}svg>foreignObject>section .rounded-1{border-radius:3px!important}svg>foreignObject>section .bg-white{background-color:#fff!important}svg>foreignObject>section .bg-gray-light{background-color:#fafbfc!important}svg>foreignObject>section .text-gray-light{color:#6a737d!important}svg>foreignObject>section .pl-3,svg>foreignObject>section .px-3{padding-left:16px!important}svg>foreignObject>section .px-3{padding-right:16px!important}svg>foreignObject>section .f6{font-size:12px!important}svg>foreignObject>section>svg>foreignObject>section section.f6{--marpit-root-font-size:12px!important}svg>foreignObject>section .lh-condensed{line-height:1.25!important}svg>foreignObject>section .text-bold{font-weight:600!important}svg>foreignObject>section .pl-c{color:#6a737d}svg>foreignObject>section .pl-c1,svg>foreignObject>section .pl-s .pl-v{color:#005cc5}svg>foreignObject>section .pl-e,svg>foreignObject>section .pl-en{color:#6f42c1}svg>foreignObject>section .pl-s .pl-s1,svg>foreignObject>section .pl-smi{color:#24292e}svg>foreignObject>section .pl-ent{color:#22863a}svg>foreignObject>section .pl-k{color:#d73a49}svg>foreignObject>section .pl-pds,svg>foreignObject>section .pl-s,svg>foreignObject>section .pl-s .pl-pse .pl-s1,svg>foreignObject>section .pl-sr,svg>foreignObject>section .pl-sr .pl-cce,svg>foreignObject>section .pl-sr .pl-sra,svg>foreignObject>section .pl-sr .pl-sre{color:#032f62}svg>foreignObject>section .pl-smw,svg>foreignObject>section .pl-v{color:#e36209}svg>foreignObject>section .pl-bu{color:#b31d28}svg>foreignObject>section .pl-ii{color:#fafbfc;background-color:#b31d28}svg>foreignObject>section .pl-c2{color:#fafbfc;background-color:#d73a49}svg>foreignObject>section .pl-c2:before{content:"^M"}svg>foreignObject>section .pl-sr .pl-cce{font-weight:700;color:#22863a}svg>foreignObject>section .pl-ml{color:#735c0f}svg>foreignObject>section .pl-mh,svg>foreignObject>section .pl-mh .pl-en,svg>foreignObject>section .pl-ms{font-weight:700;color:#005cc5}svg>foreignObject>section .pl-mi{font-style:italic;color:#24292e}svg>foreignObject>section .pl-mb{font-weight:700;color:#24292e}svg>foreignObject>section .pl-md{color:#b31d28;background-color:#ffeef0}svg>foreignObject>section .pl-mi1{color:#22863a;background-color:#f0fff4}svg>foreignObject>section .pl-mc{color:#e36209;background-color:#ffebda}svg>foreignObject>section .pl-mi2{color:#f6f8fa;background-color:#005cc5}svg>foreignObject>section .pl-mdr{font-weight:700;color:#6f42c1}svg>foreignObject>section .pl-ba{color:#586069}svg>foreignObject>section .pl-sg{color:#959da5}svg>foreignObject>section .pl-corl{text-decoration:underline;color:#032f62}svg>foreignObject>section .mb-0{margin-bottom:0!important}svg>foreignObject>section .my-2{margin-bottom:8px!important;margin-top:8px!important}svg>foreignObject>section .pl-0{padding-left:0!important}svg>foreignObject>section .py-0{padding-top:0!important;padding-bottom:0!important}svg>foreignObject>section .pl-1{padding-left:4px!important}svg>foreignObject>section .pl-2{padding-left:8px!important}svg>foreignObject>section .py-2{padding-top:8px!important;padding-bottom:8px!important}svg>foreignObject>section .pl-3{padding-left:16px!important}svg>foreignObject>section .pl-4{padding-left:24px!important}svg>foreignObject>section .pl-5{padding-left:32px!important}svg>foreignObject>section .pl-6{padding-left:40px!important}svg>foreignObject>section .pl-7{padding-left:48px!important}svg>foreignObject>section .pl-8{padding-left:64px!important}svg>foreignObject>section .pl-9{padding-left:80px!important}svg>foreignObject>section .pl-10{padding-left:96px!important}svg>foreignObject>section .pl-11{padding-left:112px!important}svg>foreignObject>section .pl-12{padding-left:128px!important}svg>foreignObject>section hr{border-bottom-color:#eee}svg>foreignObject>section kbd{display:inline-block;padding:3px 5px;font:11px SFMono-Regular,Consolas,Liberation Mono,Menlo,monospace;line-height:10px;color:#444d56;vertical-align:middle;background-color:#fafbfc;border:1px solid #d1d5da;border-radius:3px;box-shadow:inset 0 -1px 0 #d1d5da}svg>foreignObject>section:after,svg>foreignObject>section:before{display:table
/* content:""; */}svg>foreignObject>section:after{clear:both}svg>foreignObject>section>:first-child{margin-top:0!important}svg>foreignObject>section>:last-child{margin-bottom:0!important}svg>foreignObject>section a:not([href]){color:inherit;text-decoration:none}svg>foreignObject>section blockquote,svg>foreignObject>section details,svg>foreignObject>section dl,svg>foreignObject>section ol,svg>foreignObject>section p,svg>foreignObject>section pre,svg>foreignObject>section table,svg>foreignObject>section ul{margin-top:0;margin-bottom:16px}svg>foreignObject>section hr{height:.25em;padding:0;margin:24px 0;background-color:#e1e4e8;border:0}svg>foreignObject>section blockquote{padding:0 1em;color:#6a737d;border-left:.25em solid #dfe2e5}svg>foreignObject>section blockquote>:first-child{margin-top:0}svg>foreignObject>section blockquote>:last-child{margin-bottom:0}svg>foreignObject>section h1,svg>foreignObject>section h2,svg>foreignObject>section h3,svg>foreignObject>section h4,svg>foreignObject>section h5,svg>foreignObject>section h6{margin-top:24px;margin-bottom:16px;font-weight:600;line-height:1.25}svg>foreignObject>section h1{font-size:2em}svg>foreignObject>section h1,svg>foreignObject>section h2{padding-bottom:.3em;border-bottom:1px solid #eaecef}svg>foreignObject>section h2{font-size:1.5em}svg>foreignObject>section h3{font-size:1.25em}svg>foreignObject>section h4{font-size:1em}svg>foreignObject>section h5{font-size:.875em}svg>foreignObject>section h6{font-size:.85em;color:#6a737d}svg>foreignObject>section ol,svg>foreignObject>section ul{padding-left:2em}svg>foreignObject>section ol ol,svg>foreignObject>section ol ul,svg>foreignObject>section ul ol,svg>foreignObject>section ul ul{margin-top:0;margin-bottom:0}svg>foreignObject>section li{word-wrap:break-all}svg>foreignObject>section li>p{margin-top:16px}svg>foreignObject>section li+li{margin-top:.25em}svg>foreignObject>section dl{padding:0}svg>foreignObject>section dl dt{padding:0;margin-top:16px;font-size:1em;font-style:italic;font-weight:600}svg>foreignObject>section dl dd{padding:0 16px;margin-bottom:16px}svg>foreignObject>section table{display:block;width:100%;overflow:auto}svg>foreignObject>section table th{font-weight:600}svg>foreignObject>section table td,svg>foreignObject>section table th{padding:6px 13px;border:1px solid #dfe2e5}svg>foreignObject>section table tr{background-color:#fff;border-top:1px solid #c6cbd1}svg>foreignObject>section table tr:nth-child(2n){background-color:#f6f8fa}svg>foreignObject>section img{max-width:100%;box-sizing:initial;background-color:#fff}svg>foreignObject>section img[align=right]{padding-left:20px}svg>foreignObject>section img[align=left]{padding-right:20px}svg>foreignObject>section code{padding:.2em .4em;margin:0;font-size:85%;background-color:rgba(27,31,35,.05);border-radius:3px}svg>foreignObject>section pre{word-wrap:normal}svg>foreignObject>section pre>code{padding:0;margin:0;font-size:100%;word-break:normal;white-space:pre;background:transparent;border:0}svg>foreignObject>section .highlight{margin-bottom:16px}svg>foreignObject>section .highlight pre{margin-bottom:0;word-break:normal}svg>foreignObject>section pre{padding:16px;overflow:auto;font-size:85%;line-height:1.45;background-color:#f6f8fa;border-radius:3px}svg>foreignObject>section pre code{display:inline;max-width:auto;padding:0;margin:0;overflow:visible;line-height:inherit;word-wrap:normal;background-color:initial;border:0}svg>foreignObject>section .commit-tease-sha{display:inline-block;font-family:SFMono-Regular,Consolas,Liberation Mono,Menlo,monospace;font-size:90%;color:#444d56}svg>foreignObject>section>svg>foreignObject>section section.commit-tease-sha{--marpit-root-font-size:90%}svg>foreignObject>section .full-commit .btn-outline:not(:disabled):hover{color:#005cc5;border-color:#005cc5}svg>foreignObject>section .blob-wrapper{overflow-x:auto;overflow-y:hidden}svg>foreignObject>section .blob-wrapper-embedded{max-height:240px;overflow-y:auto}svg>foreignObject>section .blob-num{width:1%;min-width:50px;padding-right:10px;padding-left:10px;font-family:SFMono-Regular,Consolas,Liberation Mono,Menlo,monospace;font-size:12px;line-height:20px;color:rgba(27,31,35,.3);text-align:right;white-space:nowrap;vertical-align:top;cursor:pointer;-webkit-user-select:none;-moz-user-select:none;-ms-user-select:none;user-select:none}svg>foreignObject>section>svg>foreignObject>section section.blob-num{--marpit-root-font-size:12px}svg>foreignObject>section .blob-num:hover{color:rgba(27,31,35,.6)}svg>foreignObject>section .blob-num:before{content:attr(data-line-number)}svg>foreignObject>section .blob-code{position:relative;padding-right:10px;padding-left:10px;line-height:20px;vertical-align:top}svg>foreignObject>section .blob-code-inner{overflow:visible;font-family:SFMono-Regular,Consolas,Liberation Mono,Menlo,monospace;font-size:12px;color:#24292e;word-wrap:normal;white-space:pre}svg>foreignObject>section>svg>foreignObject>section section.blob-code-inner{--marpit-root-font-size:12px}svg>foreignObject>section .pl-token.active,svg>foreignObject>section .pl-token:hover{cursor:pointer;background:#ffea7f}svg>foreignObject>section .tab-size[data-tab-size="1"]{-moz-tab-size:1;-o-tab-size:1;tab-size:1}svg>foreignObject>section .tab-size[data-tab-size="2"]{-moz-tab-size:2;-o-tab-size:2;tab-size:2}svg>foreignObject>section .tab-size[data-tab-size="3"]{-moz-tab-size:3;-o-tab-size:3;tab-size:3}svg>foreignObject>section .tab-size[data-tab-size="4"]{-moz-tab-size:4;-o-tab-size:4;tab-size:4}svg>foreignObject>section .tab-size[data-tab-size="5"]{-moz-tab-size:5;-o-tab-size:5;tab-size:5}svg>foreignObject>section .tab-size[data-tab-size="6"]{-moz-tab-size:6;-o-tab-size:6;tab-size:6}svg>foreignObject>section .tab-size[data-tab-size="7"]{-moz-tab-size:7;-o-tab-size:7;tab-size:7}svg>foreignObject>section .tab-size[data-tab-size="8"]{-moz-tab-size:8;-o-tab-size:8;tab-size:8}svg>foreignObject>section .tab-size[data-tab-size="9"]{-moz-tab-size:9;-o-tab-size:9;tab-size:9}svg>foreignObject>section .tab-size[data-tab-size="10"]{-moz-tab-size:10;-o-tab-size:10;tab-size:10}svg>foreignObject>section .tab-size[data-tab-size="11"]{-moz-tab-size:11;-o-tab-size:11;tab-size:11}svg>foreignObject>section .tab-size[data-tab-size="12"]{-moz-tab-size:12;-o-tab-size:12;tab-size:12}svg>foreignObject>section .task-list-item{list-style-type:none}svg>foreignObject>section .task-list-item+.task-list-item{margin-top:3px}svg>foreignObject>section .task-list-item input{margin:0 .2em .25em -1.6em;vertical-align:middle}svg>foreignObject>section .hljs{display:block;background:#fff;padding:.5em;color:#333;overflow-x:auto}svg>foreignObject>section .hljs-comment,svg>foreignObject>section .hljs-meta{color:#969896}svg>foreignObject>section .hljs-emphasis,svg>foreignObject>section .hljs-quote,svg>foreignObject>section .hljs-strong,svg>foreignObject>section .hljs-template-variable,svg>foreignObject>section .hljs-variable{color:#df5000}svg>foreignObject>section .hljs-keyword,svg>foreignObject>section .hljs-selector-tag,svg>foreignObject>section .hljs-type{color:#d73a49}svg>foreignObject>section .hljs-attribute,svg>foreignObject>section .hljs-bullet,svg>foreignObject>section .hljs-literal,svg>foreignObject>section .hljs-symbol{color:#0086b3}svg>foreignObject>section .hljs-name,svg>foreignObject>section .hljs-section{color:#63a35c}svg>foreignObject>section .hljs-tag{color:#333}svg>foreignObject>section .hljs-attr,svg>foreignObject>section .hljs-selector-attr,svg>foreignObject>section .hljs-selector-class,svg>foreignObject>section .hljs-selector-id,svg>foreignObject>section .hljs-selector-pseudo,svg>foreignObject>section .hljs-title{color:#6f42c1}svg>foreignObject>section .hljs-addition{color:#55a532;background-color:#eaffea}svg>foreignObject>section .hljs-deletion{color:#bd2c00;background-color:#ffecec}svg>foreignObject>section .hljs-link{text-decoration:underline}svg>foreignObject>section .hljs-number{color:#005cc5}svg>foreignObject>section .hljs-string{color:#032f62}svg>foreignObject>section svg[data-marp-fitting=svg]{max-height:563px}svg>foreignObject>section h1{color:#246;font-size:1.6em}svg>foreignObject>section h1,svg>foreignObject>section h2{border-bottom:none}svg>foreignObject>section h2{font-size:1.3em}svg>foreignObject>section h3{font-size:1.1em}svg>foreignObject>section h4{font-size:1.05em}svg>foreignObject>section h5{font-size:1em}svg>foreignObject>section h6{font-size:.9em}svg>foreignObject>section h1 strong,svg>foreignObject>section h2 strong,svg>foreignObject>section h3 strong,svg>foreignObject>section h4 strong,svg>foreignObject>section h5 strong,svg>foreignObject>section h6 strong{font-weight:inherit;color:#48c}svg>foreignObject>section hr{height:0;padding-top:.25em}svg>foreignObject>section pre{border:1px solid #999;line-height:1.15;overflow:visible}svg>foreignObject>section pre code svg[data-marp-fitting=svg]{max-height:529px}svg>foreignObject>section footer,svg>foreignObject>section header{margin:0;position:absolute;left:30px;color:hsla(0,0%,40%,.75);font-size:18px}svg>foreignObject>section header{top:21px}svg>foreignObject>section footer{bottom:21px}svg>foreignObject>section{align-items:stretch;background:#fff;display:flex;flex-direction:column;flex-wrap:nowrap;font-size:29px;height:720px;justify-content:center;padding:78.5px;width:1280px}svg>foreignObject>section{--marpit-root-font-size:29px}svg>foreignObject>section>:last-child,svg>foreignObject>section[data-footer]>:nth-last-child(2){margin-bottom:0}svg>foreignObject>section>:first-child,svg>foreignObject>section>header:first-child+*{margin-top:0}svg>foreignObject>section:after{position:absolute;padding:0;right:30px;bottom:21px;font-size:24px;color:#777}svg>foreignObject>section:after{--marpit-root-font-size:24px}svg>foreignObject>section.invert{background-color:#222;color:#e6eaf0}svg>foreignObject>section.invert:after{color:#999}svg>foreignObject>section.invert img{background-color:transparent}svg>foreignObject>section.invert a{color:#50b3ff}svg>foreignObject>section.invert h1{color:#a3c5e7}svg>foreignObject>section.invert h2,svg>foreignObject>section.invert h3,svg>foreignObject>section.invert h4,svg>foreignObject>section.invert h5{color:#ebeff5}svg>foreignObject>section.invert blockquote,svg>foreignObject>section.invert h6{border-color:#3d3f43;color:#939699}svg>foreignObject>section.invert h1 strong,svg>foreignObject>section.invert h2 strong,svg>foreignObject>section.invert h3 strong,svg>foreignObject>section.invert h4 strong,svg>foreignObject>section.invert h5 strong,svg>foreignObject>section.invert h6 strong{color:#7bf}svg>foreignObject>section.invert hr{background-color:#3d3f43}svg>foreignObject>section.invert footer,svg>foreignObject>section.invert header{color:hsla(0,0%,60%,.75)}svg>foreignObject>section.invert code,svg>foreignObject>section.invert kbd{background-color:#111}svg>foreignObject>section.invert kbd{border-color:#666;box-shadow:inset 0 -1px 0 #555;color:#e6eaf0}svg>foreignObject>section.invert table tr{background-color:#12181d;border-color:#60657b}svg>foreignObject>section.invert table tr:nth-child(2n){background-color:#1b2024}svg>foreignObject>section.invert table td,svg>foreignObject>section.invert table th{border-color:#5b5e61}svg>foreignObject>section.invert pre{background-color:#0a0e12;border-color:#777}svg>foreignObject>section.invert pre code{background-color:transparent}svg>foreignObject>section[data-color] h1,svg>foreignObject>section[data-color] h2,svg>foreignObject>section[data-color] h3,svg>foreignObject>section[data-color] h4,svg>foreignObject>section[data-color] h5,svg>foreignObject>section[data-color] h6{color:currentColor}svg>foreignObject>section[data-marpit-advanced-background=background]{display:block!important;padding:0!important}svg>foreignObject>section[data-marpit-advanced-background=background]:after,svg>foreignObject>section[data-marpit-advanced-background=background]:before,svg>foreignObject>section[data-marpit-advanced-background=content]:after,svg>foreignObject>section[data-marpit-advanced-background=content]:before{display:none!important}svg>foreignObject>section[data-marpit-advanced-background=background]>div[data-marpit-advanced-background-container]{all:initial;display:flex;flex-direction:row;height:100%;overflow:hidden;width:100%}svg>foreignObject>section[data-marpit-advanced-background=background]>div[data-marpit-advanced-background-container][data-marpit-advanced-background-direction=vertical]{flex-direction:column}svg>foreignObject>section[data-marpit-advanced-background=background][data-marpit-advanced-background-split]>div[data-marpit-advanced-background-container]{width:var(--marpit-advanced-background-split,50%)}svg>foreignObject>section[data-marpit-advanced-background=background][data-marpit-advanced-background-split=right]>div[data-marpit-advanced-background-container]{margin-left:calc(100% - var(--marpit-advanced-background-split, 50%))}svg>foreignObject>section[data-marpit-advanced-background=background]>div[data-marpit-advanced-background-container]>figure{all:initial;background-position:center;background-repeat:no-repeat;background-size:cover;flex:auto;margin:0}svg>foreignObject>section[data-marpit-advanced-background=content],svg>foreignObject>section[data-marpit-advanced-background=pseudo]{background:transparent!important}svg>foreignObject>section[data-marpit-advanced-background=pseudo],svg[data-marpit-svg]>foreignObject[data-marpit-advanced-background=pseudo]{pointer-events:none!important}svg>foreignObject>section[data-marpit-advanced-background-split]{width:100%;height:100%}</style></head><body><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="1">
<h1>The day the build broke <img class="emoji" draggable="false" alt="😔" src="https://twemoji.maxcdn.com/2/svg/1f614.svg" data-marp-twemoji=""/></h1>
<p>The tale of a broken build and breaking changes...</p>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="2" data-marpit-fragments="5">
<h2>And so the story begins...</h2>
<ul>
<li data-marpit-fragment="1"><code>ol-ishare</code> is built automatically each time a change is committed to the repository
<ul>
<li data-marpit-fragment="2">Formatting of source code is checked (linting)</li>
<li data-marpit-fragment="3">Unit tests are ran</li>
<li data-marpit-fragment="4">Library and apps such as LiteMap, Spotlight etc. are built</li>
<li data-marpit-fragment="5">Examples and apps are deployed to S3</li>
</ul>
</li>
</ul>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="3">
<h3>Behold a broken build!</h3>
<pre><code><svg data-marp-fitting="svg" data-marp-fitting-code><foreignObject><span data-marp-fitting-svg-content><span data-marp-fitting-svg-content-wrap>Test Suites: 2 failed, 9 passed, 11 total
Tests: 9 failed, 116 passed, 125 total
Snapshots: 19 passed, 19 total
Time: 28.249 s
</span></span></foreignObject></svg></code></pre>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="4">
<h3>Digging in</h3>
<p>The failed tests were testing functionality that hasn't changed recently in <code>ol-ishare</code> so must be due to something else changing.</p>
<pre><code><svg data-marp-fitting="svg" data-marp-fitting-code><foreignObject><span data-marp-fitting-svg-content><span data-marp-fitting-svg-content-wrap>● getFeatureByValue › Returns features when request is successful and features are found
TypeError: Cannot read property 'PropertyIsEqualTo' of undefined
449 | */
450 | function getFeatureByValue(owsUrl, layerName, column, value, callback) {
&gt; 451 | var filter = writeFilter(equalTo(column, value));
| ^
452 | getFeature(owsUrl, layerName, filter, null, callback);
453 | }
454 |
at serialize (node_modules/ol/src/xml.js:491:9)
at pushSerializeAndPop (node_modules/ol/src/xml.js:535:3)
at writeFilterCondition (node_modules/ol/src/format/WFS.js:1067:3)
at writeFilter (node_modules/ol/src/format/WFS.js:1339:3)
at Object.getFeatureByValue (src/esm/info.js:451:16)
at Object.&lt;anonymous&gt; (test/info.test.js:327:10)
</span></span></foreignObject></svg></code></pre>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="5" data-marpit-fragments="5">
<h3>More digging</h3>
<ul>
<li data-marpit-fragment="1">The issue was eventually narrowed down to a change in <code>ol/format/WFS~writeFilter</code> function</li>
<li data-marpit-fragment="2">The build installed <code>[email protected]</code> while we had been using the previous release <code>[email protected]</code></li>
<li data-marpit-fragment="3"><code>[email protected]</code> had introduced support for WFS 2.0
<ul>
<li data-marpit-fragment="4"><code>writeFilter</code> had been updated to include a WFS <code>version</code> argument
<ul>
<li data-marpit-fragment="5">due to how an OGC Filter is written to XML changed in WFS 2.0</li>
</ul>
</li>
</ul>
</li>
</ul>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="6">
<h3>Behold a fix!</h3>
<p>The fix was very straightforward, specify the WFS version we're using:</p>
<pre><code class="language-diff"><svg data-marp-fitting="svg" data-marp-fitting-code><foreignObject><span data-marp-fitting-svg-content><span data-marp-fitting-svg-content-wrap><span class="hljs-comment">diff --git a/src/esm/info.js b/src/esm/info.js</span>
<span class="hljs-comment">index 908f89d..34e7260 100644</span>
<span class="hljs-comment">--- a/src/esm/info.js</span>
<span class="hljs-comment">+++ b/src/esm/info.js</span>
<span class="hljs-meta">@@ -448,7 +448,7 @@</span> function getFeature(owsUrl, layerName, filter, params, callback) {
* @param {module:ol-ishare/info~getFeatureCallback} callback Function to be called with features...
*/
function getFeatureByValue(owsUrl, layerName, column, value, callback) {
<span class="hljs-deletion">- var filter = writeFilter(equalTo(column, value));</span>
<span class="hljs-addition">+ var filter = writeFilter(equalTo(column, value), &#x27;1.0.0&#x27;);</span>
getFeature(owsUrl, layerName, filter, null, callback);
}
</span></span></foreignObject></svg></code></pre>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="7" data-marpit-fragments="2">
<h3>We could of walked away at this point</h3>
<ul>
<li data-marpit-fragment="1">We've identified the issue and fixed it <img class="emoji" draggable="false" alt="✔️" src="https://twemoji.maxcdn.com/2/svg/2714.svg" data-marp-twemoji=""/></li>
<li data-marpit-fragment="2">But it's still an issue for everyone else using OpenLayers <img class="emoji" draggable="false" alt="❌" src="https://twemoji.maxcdn.com/2/svg/274c.svg" data-marp-twemoji=""/></li>
</ul>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="8" data-marpit-fragments="5">
<h3>Leave it better than you found it <img class="emoji" draggable="false" alt="🌳" src="https://twemoji.maxcdn.com/2/svg/1f333.svg" data-marp-twemoji=""/></h3>
<ul>
<li data-marpit-fragment="1">Adding a mandatory argument to an existing function is a breaking change
<ul>
<li data-marpit-fragment="2">It's a tiny change but significant</li>
</ul>
</li>
<li data-marpit-fragment="3">My original intention was a small Pull Request to make the <code>version</code> argument optional in OpenLayers to avoid a breaking change
<ul>
<li data-marpit-fragment="4">Add a test for the change</li>
<li data-marpit-fragment="5">Make the change</li>
</ul>
</li>
</ul>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="9" data-marpit-fragments="3">
<h3>There's more... <img class="emoji" draggable="false" alt="🐃" src="https://twemoji.maxcdn.com/2/svg/1f403.svg" data-marp-twemoji=""/></h3>
<ul>
<li data-marpit-fragment="1">There was no test for calling <code>writeFilter</code> with the <code>version</code> set to <code>2.0.0</code>
<ul>
<li data-marpit-fragment="2">So I added a test, which failed <img class="emoji" draggable="false" alt="💥" src="https://twemoji.maxcdn.com/2/svg/1f4a5.svg" data-marp-twemoji=""/></li>
</ul>
</li>
<li data-marpit-fragment="3">Luckily it was an easy fix</li>
</ul>
<p>Pull Request with tests and changes: <a href="https://github.com/openlayers/openlayers/pull/11890">https://github.com/openlayers/openlayers/pull/11890</a></p>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="10" data-marpit-fragments="1">
<h3>OpenLayers v6.5.0 should of been v7.0.0</h3>
<ul>
<li data-marpit-fragment="1">When a breaking change is introduced to a public API the Major version should be increase</li>
</ul>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="11" data-marpit-fragments="4">
<h3>Semantic Versioning</h3>
<p>Given a version number MAJOR.MINOR.PATCH, increment the:</p>
<ul>
<li data-marpit-fragment="1">MAJOR version when you make incompatible API changes,</li>
<li data-marpit-fragment="2">MINOR version when you add functionality in a backwards compatible manner, and</li>
<li data-marpit-fragment="3">PATCH version when you make backwards compatible bug fixes.</li>
<li data-marpit-fragment="4">Optional PRE-RELEASE version for beta, alpha, release candidate etc.</li>
</ul>
<pre><code><svg data-marp-fitting="svg" data-marp-fitting-code><foreignObject><span data-marp-fitting-svg-content><span data-marp-fitting-svg-content-wrap> 6.5.0-beta
| | | |
MAJOR version --+ | | |
MINOR version ----+ | |
PATCH version ------+ |
PRE-RELEASE version ----+
</span></span></foreignObject></svg></code></pre>
<p>Full spec: <a href="https://semver.org/">https://semver.org/</a></p>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="12" data-marpit-fragments="4">
<h3>Semantic Versioning cont.</h3>
<ul>
<li data-marpit-fragment="1">Important for APIs</li>
<li data-marpit-fragment="2">Very well established convention</li>
<li data-marpit-fragment="3">But not everyone sticks to it...</li>
<li data-marpit-fragment="4">Traditionally people treat MAJOR version changes to be a release with <strong>significant</strong> new functionality</li>
</ul>
</section>
</foreignObject></svg><svg data-marpit-svg="" viewBox="0 0 1280 720"><foreignObject width="1280" height="720"><section id="13" data-marpit-fragments="4">
<h3>Specifying versions in package.json</h3>
<p>Dependencies are defined in <code>package.json</code></p>
<pre><code class="language-json"><svg data-marp-fitting="svg" data-marp-fitting-code><foreignObject><span data-marp-fitting-svg-content><span data-marp-fitting-svg-content-wrap><span class="hljs-string">&quot;dependencies&quot;</span>: {
<span class="hljs-attr">&quot;ol&quot;</span>: <span class="hljs-string">&quot;~6.5.0&quot;</span>,
<span class="hljs-attr">&quot;ol-layerswitcher&quot;</span>: <span class="hljs-string">&quot;^3.8.1&quot;</span>,
<span class="hljs-attr">&quot;ol-popup&quot;</span>: <span class="hljs-string">&quot;&gt;=4.0.0&quot;</span>,
...
},
</span></span></foreignObject></svg></code></pre>
<p>There are number of ways to specify an acceptable version:</p>
<ul>
<li data-marpit-fragment="1">Exact version <code>6.5.0</code></li>
<li data-marpit-fragment="2">Same major range <code>^6.5.0</code> would allow <code>6.5.1</code>, <code>6.6.0</code> but not <code>7.0.0</code></li>
<li data-marpit-fragment="3">Same minor range <code>~6.5.0</code> would allow <code>6.5.1</code>, <code>6.5.2</code> but not <code>6.6.0</code></li>
<li data-marpit-fragment="4">A range of stable versions <code>&gt;</code>, <code>&lt;</code>, <code>=</code>, <code>&gt;=</code> or <code>&lt;=</code> for comparisons, or <code>-</code> to specify an inclusive range</li>
</ul>
<p>Further details at: <a href="https://semver.npmjs.com/">https://semver.npmjs.com/</a></p>
</section>
<script>!function(){"use strict";const t="marpitSVGPolyfill:setZoomFactor,",e=Symbol();let r,o;function n(n){const i="object"==typeof n&&n.target||document,a="object"==typeof n?n.zoom:n;window[e]||(Object.defineProperty(window,e,{configurable:!0,value:!0}),window.addEventListener("message",(({data:e,origin:r})=>{if(r===window.origin)try{if(e&&"string"==typeof e&&e.startsWith(t)){const[,t]=e.split(","),r=Number.parseFloat(t);Number.isNaN(r)||(o=r)}}catch(t){console.error(t)}})));let l=!1;Array.from(i.querySelectorAll("svg[data-marpit-svg]"),(t=>{var e,n,i,s;t.style.transform||(t.style.transform="translateZ(0)");const c=a||o||t.currentScale||1;r!==c&&(r=c,l=c);const d=t.getBoundingClientRect(),{length:u}=t.children;for(let r=0;r<u;r+=1){const o=t.children[r],a=o.getScreenCTM();if(a){const t=null!==(n=null===(e=o.x)||void 0===e?void 0:e.baseVal.value)&&void 0!==n?n:0,r=null!==(s=null===(i=o.y)||void 0===i?void 0:i.baseVal.value)&&void 0!==s?s:0,l=o.firstChild,{style:u}=l;u.transformOrigin||(u.transformOrigin=`${-t}px ${-r}px`),u.transform=`scale(${c}) matrix(${a.a}, ${a.b}, ${a.c}, ${a.d}, ${a.e-d.left}, ${a.f-d.top}) translateZ(0.0001px)`}}})),!1!==l&&Array.from(i.querySelectorAll("iframe"),(({contentWindow:e})=>{null==e||e.postMessage(`${t}${l}`,"null"===window.origin?"*":window.origin)}))}r=1,o=void 0;const i=(t,e,r)=>{if(t.getAttribute(e)!==r)return t.setAttribute(e,r),!0};function a({once:t=!1,target:e=document}={}){const r="Apple Computer, Inc."===navigator.vendor?[n]:[];let o=!t;const a=()=>{for(const t of r)t({target:e});!function(t=document){Array.from(t.querySelectorAll('svg[data-marp-fitting="svg"]'),(t=>{var e;const r=t.firstChild,o=r.firstChild,{scrollWidth:n,scrollHeight:a}=o;let l,s=1;if(t.hasAttribute("data-marp-fitting-code")&&(l=null===(e=t.parentElement)||void 0===e?void 0:e.parentElement),t.hasAttribute("data-marp-fitting-math")&&(l=t.parentElement),l){const t=getComputedStyle(l),e=Math.ceil(l.clientWidth-parseFloat(t.paddingLeft||"0")-parseFloat(t.paddingRight||"0"));e&&(s=e)}const c=Math.max(n,s),d=Math.max(a,1),u=`0 0 ${c} ${d}`;i(r,"width",`${c}`),i(r,"height",`${d}`),i(t,"preserveAspectRatio",getComputedStyle(t).getPropertyValue("--preserve-aspect-ratio")||"xMinYMin meet"),i(t,"viewBox",u)&&t.classList.toggle("__reflow__")}))}(e),o&&window.requestAnimationFrame(a)};return a(),()=>{o=!1}}const l=Symbol(),s=document.currentScript;((t=document)=>{if("undefined"==typeof window)throw new Error("Marp Core's browser script is valid only in browser context.");if(t[l])return t[l];const e=a({target:t}),r=()=>{e(),delete t[l]};Object.defineProperty(t,l,{configurable:!0,value:r})})(s?s.getRootNode():document)}();
</script></foreignObject></svg></body></html>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment