Skip to content

Fix asconfig merge order #1406

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jul 24, 2020
Merged

Fix asconfig merge order #1406

merged 9 commits into from
Jul 24, 2020

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Jul 22, 2020

Fixes the merge order part of #1404 by updating to the originally intended order

  1. CLI arguments (without defaults)
  2. ASConfig target
  3. ASConfig options
  4. Repeat 2. and 3. while ASConfig extends
  5. Add defaults

while moving the defaults for target and config to asc itself because we need these early. Previously, the initial invocation of parse missed to omit populating defaults, and #1404 also seems to populate these too early.

  • I've read the contributing guidelines

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 22, 2020

Hmm, this is failing in apparently unrelated packages tests now, specifically b.

@willemneal
Copy link
Contributor

I had the same issue when I tried to delay the adding defaults to args, now opt. Is the plan to merge this first and then mine? Why not just edit mine directly? You can push to my branch.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 22, 2020

Really just trying to dig into merge order in separation here, as that's what I'm primarily concerned about. Wasn't expecting unrelated breakage, though, so if you'd like to incorporate the merge order (this PR has the correct one now) into your PR, feel free to do so.

@willemneal willemneal mentioned this pull request Jul 22, 2020
Closed
willemneal
willemneal reviewed Jul 22, 2020
View reviewed changes
willemneal
willemneal reviewed Jul 22, 2020
View reviewed changes
willemneal
willemneal reviewed Jul 23, 2020
View reviewed changes
cli/asc.js Outdated
seenAsconfig.add(filePath);
asconfig = getAsconfig(fileName, asconfigDir, readFile);
}
asconfigPath = optionsUtil.resolvePath(asconfig.extends, asconfigDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I was going to mention that we should allow for node_resolution here like ts does.

@willemneal
Copy link
Contributor

#1408 Adds tests to this PR.

@dcodeIO dcodeIO mentioned this pull request Jul 24, 2020
Closed
MaxGraey
MaxGraey reviewed Jul 24, 2020
View reviewed changes
@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 24, 2020

Merging to get this fixed, but can't merge #1408 on top of it because it includes #1411 for some reason (probably a mistake). Suggesting to rebase #1408 cleanly on top of master afterwards.

@dcodeIO dcodeIO merged commit df40407 into master Jul 24, 2020
@github-actions
Copy link

🎉 This PR is included in version 0.14.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dcodeIO dcodeIO deleted the issue-1404 branch June 1, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxGraey MaxGraey MaxGraey left review comments

@willemneal willemneal willemneal left review comments

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dcodeIO @willemneal @MaxGraey