-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Conversation
lib/selenium-webdriver/webdriver.js
Outdated
@@ -236,7 +236,7 @@ webdriver.WebDriver.prototype.executeAsyncScript = (script, var_args) => {}; | |||
* button.click(); | |||
* browser.call(logText, counter); | |||
* | |||
* @param {function(...): (T|promise.Promise<T>)} fn The function to | |||
* @param {function(...*): (T|promise.Promise<T>)} fn The function to |
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.
If this change is not related to the banner update, I'd recommend reverting back. I assume you're not able to build without it?
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.
Happy to do so! Yeah, I'm a little concerned about the build process to actually deploy this as there were a few docs / build scripts that felt like they may be half-updated. I was able to get it to build, but it required updating the dgeni
version for a bug fix (and thus where this came in)
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 recommend squashing these commits, as the last two don't provide a lot of value and just fixup stuff from the first.
Wording of the messages sounds good to me, just one note about the website lifetime.
website/README.md
Outdated
cd website/ | ||
npm install | ||
npm run build | ||
npx gulp |
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.
Can you include the version of gulp
you're using for future reference? That way updates to gulp
don't break this process.
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 believe npx gulp
will first look in the node_modules/.bin/
first, so it should pin directly to whatever's listed in the package.json
. That said, happy to include the specific version if preferable!
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.
Oh I didn't realize that was properly listed as a dev dependency. How about we make "gulp": "gulp"
a script and then document it as npm run gulp
. That way it's specifically tied to the installed dependency, not downloaded over the network (ex. if the user forgot to npm install
first).
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.
Great call! I had totally missed it, but npm run build
would essentially do just that
website/README.md
Outdated
#FailureMessage Object: 0x306a626c0 | ||
``` | ||
|
||
Eventually the project should migrate to Gulp 5 / Node 18+ to solve this issue, but in the meantime a simple workaround to run `npm run arm64_repair` (after every `npm install` in the `website/` directory) |
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.
Should this be included in the "How to run" instructions above?
website/partials/status.html
Outdated
</div> | ||
|
||
<h2>What does End Of Life mean?</h2> | ||
The packages will remain accessible on GitHub, npm, Bower, and Release archive. This website will also remain here indefinitely. The project will be archived on GitHub and remain accessible indefinitely, but will no longer be open to issues / comments / etc. |
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.
This website will also remain here indefinitely.
@mgechev, are we comfortable committing to the website staying up forever? I doubt GitHub Pages would push a significant breaking change here, but if they did and it was a non-trivial effort to move off it, I expect we probably wouldn't bother given Internet Archive as the ultimate workaround.
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.
Yeah, I agree. Let's change to "foreseeable future"
We're not anticipating making the website unavailable, but inevitably it'll happen one day.
@dgp1130 I think we're good to go. I squashed to a single commit and cleaned up the issues, but definitely let me know if there's anything else we can do on our end! Thanks |
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.
Yep, looks great. Thanks @dwelch2344!
I'll take a look at building and pushing to |
Sorry, got distracted with v16. Pushed the release now and protractortest.org seems to be updated correctly. I needed to add back in that typing change to get it to build, not sure how you were avoiding that. I also needed to swap a boolean condition as it seems the header message was displayed in the wrong order. |
With Protractor becoming LTS and given the partnership with HeroDevs to provide support past August 2023, we had discussed adding documentation to explain what the future holds.
The bulk of this is adding a
banner
to every page and astatus page
that gives more information about EOL. Both of these have 3 minor variants of verbiage, depending on the browser's date on render. Screenshots below:Now - Aug 2023
Sept 2023 - Aug 2024
Sept 2024+
Note: This PR replaces #5557 (and #5556) to both address EOL and update the docs / builds for the website. Apologies for another PR, but it looks like the current site is currently running off a 5.4 commit that never made it to the release branch.