search

immersive-web / webxr-polyfill

Use the WebXR Device API today, providing fallbacks to native WebVR 1.1 and Cardboard
Apache License 2.0
378 stars 84 forks source link

small fix to XRSession.end() #131

Closed blairmacintyre closed 4 years ago

blairmacintyre commented 4 years ago

typo: this.immersive should be this[PRIVATE].immersive

jsantell commented 4 years ago

Once the build flies are removed from the commit, LGTM!

blairmacintyre commented 4 years ago

Oh, we don’t include the builds? How do they get rebuilt?

Blair MacIntyre Principal Research Scientist

snt frm iOS kybrd, pls pardon typos and weird auto-corrections

blairmacintyre.me && pronoun.is/he/him

From: Jordan Santell notifications@github.com Sent: Wednesday, January 1, 2020 5:43 PM To: immersive-web/webxr-polyfill Cc: Blair MacIntyre; Author Subject: Re: [immersive-web/webxr-polyfill] small fix to XRSession.end() (#131)

Once the build flies are removed from the commit, LGTM!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/immersive-web/webxr-polyfill/pull/131?email_source=notifications&email_token=AAKJJ7PTBW7MKJJZD3DTQCLQ3UMBHA5CNFSM4KACPRC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH5OBXA#issuecomment-570089692, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAKJJ7NAR2OXG76ABT3WJBLQ3UMBHANCNFSM4KACPRCQ.

jsantell commented 4 years ago

Oh, we don’t include the builds? How do they get rebuilt?

As a separate commit -- generally I don't merge any minified/artifact code since it's unverifiable with possibly different dependencies or changes, or worse, malicious code. Of course I trust you Blair :smile: and it's a great question that hasn't been revisited since this project was very much in its infancy. While I still don't think PRs should be accepted with built artifacts for that reason, building becomes a bottleneck (someone has to build/commit manually), and now that Github actions are available, it might be a good time to revisit. An action could build/commit on every commit if that's desirable, or possibly is it still necessary to commit an artifact to git history, etc.

I have a slight preference towards only committing the artifact on version releases, but haven't thought much about it, and publishing practices seem to change frequently.

Edit: PRs with builds also make it more difficult to merge multiple PRs from a similar point in history

blairmacintyre commented 4 years ago

Totally makes sense on the builds.

Anyway, I removed them.

jsantell commented 4 years ago

Thank you @blairmacintyre! I'll open up an issue for discussion on some sort of build automation w/r/t the current topic as well