Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

add windows core package detection #307

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Aerijo
Copy link
Contributor

@Aerijo Aerijo commented Jul 14, 2020

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

On Windows the snippet file paths use \ as the delimiter. This doesn't match the /node_modules/ regex, so community snippets don't get priority.

The change sets the regex depending on if the OS is Windows or not.

Root cause

  • Last known to pass on 1.46.0-beta0 and 1.45.0 (Mar 11 2020, Add CodeQL Analysis workflow #305), but break on 1.48.0 (Jun 10 2020, Make snippet parsing and expansion lazy #306).
  • The package paths come from the package-manager module in Atom, which creates the list using an UnderscoreJS object keys to array method, which is ultimately a call to Object.keys.
  • Notable changes: Electron 4.2.7 -> 5.0.12
  • Corresponds to Node 10.11.0 -> 12.0.0

I don't have the patience to work out exactly what caused the issue, but a jump in Node means something could have happened to the Object.keys behaviour, the order of which is unspecified for those versions.

Alternate Designs

None

Benefits

Priority test passes

Possible Drawbacks

I don't know why this issue suddenly appeared. It came up in #306, but the cause does not seem to be related to that PR. The check itself has existed (in similar form) for years. The path comes from the Atom package manager module, so perhaps it used to normalise them? Or the files just happened to be in the right order other times?

Applicable Issues

None

@Aerijo
Copy link
Contributor Author

Aerijo commented Jul 31, 2020

The docs have isBundledPackage as a method on the package manager. It may be better to replace the regex test entirely, and use the builtin method instead. I just don't know if using it has already been considered at some point and found to be lacking.

Copy link
Contributor

@savetheclocktower savetheclocktower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confess to not having looked at this PR until just now, when I realized it'd solve the issue that my PR has with passing CI.

Absent any specific discussion where isBundledPackage was considered in the past and rejected, I've got no problem with using it to solve this problem. But the regex test is an improvement over the status quo, so I'd also be cool with leaving it as-is if you don't want to change it for whatever reason.

I left one unrelated question, but I'll be happy to 👍 this once it's answered.

@@ -5,6 +5,9 @@ notifications:
on_success: never
on_failure: change

env:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar enough with the Atom toolchain to know this, so I'll ask: what's the significance of adding this? Is it necessary to fix this issue?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants