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

lazy load `CustomEvent` to fix snapshotting #1384

Merged
merged 4 commits into from Apr 11, 2018

Conversation

Projects
None yet
2 participants
@annthurium
Contributor

annthurium commented Apr 11, 2018

Problem: When I went to prepare the 0.13.0 release, snapshotting failed with the following output:

Generating snapshot script at "/Users/distiller/atom/out/startup.js" (483)
Verifying if snapshot can be executed via `mksnapshot`
/Users/distiller/atom/out/startup.js:294944
      let FakeKeyDownEvent = class FakeKeyDownEvent extends CustomEvent {
                                                            ^
ReferenceError: CustomEvent is not defined
    at Object.../node_modules/github/lib/views/commit-view.js (/Users/distiller/atom/out/startup.js:294944:61)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
    at Object.../node_modules/github/lib/controllers/commit-controller.js (/Users/distiller/atom/out/startup.js:279080:25)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
    at Object.../node_modules/github/lib/views/git-tab-view.js (/Users/distiller/atom/out/startup.js:232123:31)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
    at Object.../node_modules/github/lib/controllers/git-tab-controller.js (/Users/distiller/atom/out/startup.js:189673:25)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
    at Object.../node_modules/github/lib/controllers/root-controller.js (/Users/distiller/atom/out/startup.js:104740:31)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
Error: Command failed: /Users/distiller/atom/out/Atom.app/Contents/MacOS/Atom /Users/distiller/atom/script/verify-snapshot-script /Users/distiller/atom/out/startup.js
/Users/distiller/atom/out/startup.js:294944
      let FakeKeyDownEvent = class FakeKeyDownEvent extends CustomEvent {

CustomEvent is a DOM primitive that is not available in the v8 context. Solution is to make the code that needs CustomEvent not execute while snapshotting.

Test plan:

  • Ran unit tests for atom/github
  • Manually tested adding a co author and clearing the co author suggested inputs with esc key, which uses the FakeKeyDown functionality.
  • Published a pre release version of atom/github, and then built atom/atom locally with my pre release version. Build output successfully executed snapshotting.

annthurium added some commits Apr 11, 2018

@annthurium annthurium merged commit 4021cef into master Apr 11, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annthurium annthurium deleted the tt-18-apr-fix-snapshot branch Apr 11, 2018

annthurium added a commit to atom/atom that referenced this pull request Apr 11, 2018

bump atom/github to 0.13.2
Fixed snapshotting issue in atom/github#1384
@@ -78,7 +83,7 @@ export default class CommitView extends React.Component {
return;
}
const fakeEvent = new FakeKeyDownEvent(keyCode);
const fakeEvent = getFakeKeyDownEvent(keyCode);

This comment has been minimized.

@kuychaco

kuychaco Apr 11, 2018

Member

Sweet, thanks for fixing the broken snapshotting @annthurium!

One minor suggestion with efficiency in mind - with this approach we are creating a new class FakeKeyDownEvent every time proxyKeyCode gets called.

We can do this just once if we do something like this:

have a global variable:

let FakeKeyDownEvent;

and in proxyKeyCode check to see if it exists, creating it if it doesn't:

if (!FakeKeyDownEvent) {
  FakeKeyDownEvent = class FakeKeyDownEvent extends CustomEvent {
    constructor(kCode) {
      super('keydown');
      this.keyCode = kCode;
    }
  }
}

const fakeEvent = new FakeKeyDownEvent(keyCode);

This comment has been minimized.

@annthurium

annthurium Apr 11, 2018

Contributor

good suggestion! Will fix.

dolsenj added a commit to dolsenj/atom that referenced this pull request Apr 20, 2018

Added code to perform tab detection when a pane is changed. This is u…
…seful when the tab length of a file differs from the default tab language for a programming language.

Fixed lint errors.

Added spec tests to workspace-spec because there are currently no tests for the tab length detection change. The JavaScript sample files are test inputs for tab length detection.

Only changes the tab length when detected tab length differs from current tab length because this change was reverted.

Reverting back to commit because future commits caused build errors.

⬆️ tree-sitter and language packages

⬆️ text-buffer

Address review feedback

Add exemptLabels to the configuration

Limit the bot to issues only

Capitalize Safe Mode

Revert "⬆️ text-buffer"

This reverts commit 553dec3.

⬆️ line-ending-selector

⬆️ text-buffer

Take 2

Special-casing \r breaks the word regexp

⬆️ find-and-replace

Remove duplicate command bindings for undo and redo

Co-authored-by: Nathan Sobo <nathan@github.com>

Remove JSX

Defer loading of PaneContainerElement to fix snapshotting

⬆️ fuzzy-finder@1.8.1

@smashwilson's updates

Add Teletype highlights from the past week and focus for the coming week

Defer component initialization until element is requested

This fixes the error snapshotting Docks described in atom#16864.

⬆️ tree-view@0.223.0

Revert "⬆️ tree-view@0.223.0"

This reverts commit 139b3c6.

⬆️ one-dark/light-ui@v1.12.0

📝 Document deprecation for `undo` option

Refs atom#16956

⬆️ text-buffer@13.14.0

⬆️ tree-sitter

Weekly focus document template

@smashwilson's update for last week.

More sparse than I like to see 😞

add @annthurium's contributions from last wek

Fix markdown formatting

Fix link

Add Teletype highlights from the past week

Add reactor duty highlights from the past week

Add @daviwil focus for the week

Add Max's focus

Add Xray things

⬆️ autocomplete-plus@2.40.6

⬆️ find-and-replace@0.215.8

⬆️ tree-sitter

⬆️ one-dark/light-ui@v1.12.1

⬆️ find-and-replace

Bump github package version to 0.13.0

bump atom/github to 0.13.2

Fixed snapshotting issue in atom/github#1384

⬆️ spell-check@0.73.2

⬆️ snippets@1.3.2

⬆️ styleguide@v0.49.11

⬆️ text-buffer

⬆️ github

⬆️ spell-check@0.73.3

⬆️ language-python

⬆️ grammar-selector@0.50.1

Separate TextEditor keyboard enablement from readOnly state

Derive the `readonly` attribute from `model.isReadOnly()`

Return the property we're actually setting

Can't use the same name for the property and method

Boolean logic, my old enemy

Increment the version of the readOnly serialization key

👕 restore missing break

Adding additional operating systems to travis.yml because there are issues when multiple operating systems are used.

Changes the tab length only when it is different from the current tab length because it is redundant to set the tab length to itself.

Fixed lint errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment