Skip to content

No native git dependency #1003

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
Closed
wants to merge 5 commits into from
Closed

No native git dependency #1003

wants to merge 5 commits into from

Conversation

@DavidGOrtega
Copy link
Contributor

@DavidGOrtega DavidGOrtega commented May 11, 2022 •
edited
Loading

Removes native git dep. This effort tries to:

  • avoids future breakage due to git changes
  • avoids having to manage multiple git versions within the code

This effort introduces init function in CML. This might be a breaking change for anyone that is using CML as a lib opposed to the common CLI

@DavidGOrtega DavidGOrtega temporarily deployed to internal May 11, 2022 20:11 Inactive
@DavidGOrtega DavidGOrtega marked this pull request as draft May 11, 2022 20:11
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented May 11, 2022

This might be a breaking change for anyone that is using CML as a lib opposed to the common CLI

I guess that this breaking change will be the only way to know if that's the case; I doubt it is. 😄

@DavidGOrtega DavidGOrtega temporarily deployed to internal May 11, 2022 20:14 Inactive
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented May 11, 2022

Coining a new concept: fault–induced telemetry.

@DavidGOrtega DavidGOrtega temporarily deployed to internal May 11, 2022 20:20 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal May 11, 2022 21:29 Inactive
@DavidGOrtega DavidGOrtega changed the title No native git dependancy No native git dependency May 11, 2022
@DavidGOrtega DavidGOrtega temporarily deployed to internal May 12, 2022 10:48 Inactive
@casperdcl casperdcl added technical-debt Refactoring, linting & tidying p2-nice-to-have Low priority labels May 12, 2022
@casperdcl
Copy link
Contributor

casperdcl commented May 12, 2022

breaking change for anyone that is using CML as a lib

I wouldn't call this a breaking change since "CML as a lib" is not documented so not (yet) a public API

@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented May 12, 2022

I would not consider it p2, I would say p1 to avoid several disasters like in the past

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented May 12, 2022

to avoid several disasters like in the past

...and [potentially] allow for different ones. 😄

0x2b3bfa0
0x2b3bfa0 reviewed May 12, 2022
View reviewed changes
src/cml.js
throw new Error(`driver ${driver} unknown!`);
};

const fixGitSafeDirectory = () => {
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 May 12, 2022

Choose a reason for hiding this comment

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

This also solves the permission issues for any git commands ran by users. Should it be ported to the new library?

Copy link
Contributor Author

@DavidGOrtega DavidGOrtega May 13, 2022

Choose a reason for hiding this comment

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

Im testing it. ideally should not be the case. We do not depend on git.

@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented May 13, 2022

Im right now stuck on making the build with pkg. This is the main guilty
🙁

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented May 13, 2022 •
edited
Loading

It's a tough one. Have you considered using Babel to translate all the ESM imports into CJS ones?

@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented May 23, 2022

It's a tough one. Have you considered using Babel to translate all the ESM imports into CJS ones?

I missed this comment.
Yes I tried. I tried to use cjs but its not working at all. Its a kind of a nightmare

@0x2b3bfa0 0x2b3bfa0 mentioned this pull request Jun 8, 2022
Closed
@DavidGOrtega DavidGOrtega mentioned this pull request Jun 9, 2022
Merged
8 tasks
@dacbd dacbd mentioned this pull request Jun 10, 2022
Closed
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Jun 14, 2022

As per #1051 (comment), and lacking vercel/pkg#1323, this pull request will have to stay on CJS unless you find a better workaround. 😅

@0x2b3bfa0 0x2b3bfa0 mentioned this pull request Jun 16, 2022
Closed
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Jun 16, 2022

Note that this pull request should ideally fix #628 (comment)

@0x2b3bfa0 0x2b3bfa0 added blocked Dependent on something else and removed p2-nice-to-have Low priority labels Sep 6, 2022
@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 15 times, most recently from eb1e5a3 to 9e8bdf4 Compare September 12, 2022 19:59
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Sep 13, 2022 •
edited
Loading

@DavidGOrtega & @iterative/cml in general: this pull request is probably going to be blocked for a long time, because #1051 (comment) is going to take a couple eons to be released. Should we leave it open until then?

@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 5 times, most recently from 7243fd2 to 1e081f3 Compare September 30, 2022 04:22
@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 3 times, most recently from e2bed3a to fe9a06f Compare October 8, 2022 04:28
@0x2b3bfa0 0x2b3bfa0 mentioned this pull request Jan 12, 2023
Closed
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Feb 21, 2023

Closing unmerged after leaving a note at #1051 (comment)

@0x2b3bfa0 0x2b3bfa0 closed this Feb 21, 2023
@0x2b3bfa0 0x2b3bfa0 deleted the feat/no-git-dep branch February 21, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0x2b3bfa0 0x2b3bfa0 0x2b3bfa0 left review comments

Assignees

@DavidGOrtega DavidGOrtega

Labels

blocked Dependent on something else technical-debt Refactoring, linting & tidying

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@DavidGOrtega @0x2b3bfa0 @casperdcl