Skip to content
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

Bump uri template #619

Merged
merged 7 commits into from
Jun 12, 2023
Merged

Bump uri template #619

merged 7 commits into from
Jun 12, 2023

Conversation

andreaTP
Copy link
Contributor

This should fix #584 , I couldn't verify that this completely solves the problem but is a good guess ...

@andreaTP andreaTP temporarily deployed to build_test April 26, 2023 11:49 — with GitHub Actions Inactive
@andreaTP andreaTP temporarily deployed to build_test April 26, 2023 11:49 — with GitHub Actions Inactive
@andreaTP andreaTP temporarily deployed to build_test April 26, 2023 11:49 — with GitHub Actions Inactive
edewit
edewit previously approved these changes Apr 26, 2023
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution as always.
We use yarn here and lerna, would you mind dropping the package lock and running a yarn install for sanity please? Also left a couple of comments.

baywet
baywet previously approved these changes Apr 26, 2023
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

@koros for final review and testing in a browser setup + in a node app setup

@andreaTP
Copy link
Contributor Author

should be ready now 🙂

baywet
baywet previously approved these changes Apr 26, 2023
@andreaTP andreaTP temporarily deployed to build_test April 26, 2023 13:58 — with GitHub Actions Inactive
@andreaTP andreaTP temporarily deployed to build_test April 26, 2023 13:58 — with GitHub Actions Inactive
@andreaTP andreaTP temporarily deployed to build_test April 26, 2023 13:58 — with GitHub Actions Inactive
@andreaTP
Copy link
Contributor Author

andreaTP commented May 22, 2023

Rebased on latest main, @koros can you, please, trigger the CI? 🙏

@andreaTP andreaTP temporarily deployed to build_test May 22, 2023 10:09 — with GitHub Actions Inactive
@andreaTP andreaTP temporarily deployed to build_test May 22, 2023 10:09 — with GitHub Actions Inactive
@andreaTP andreaTP temporarily deployed to build_test May 22, 2023 10:09 — with GitHub Actions Inactive
@koros
Copy link
Contributor

koros commented May 22, 2023

@andreaTP there's something else broken but it has nothing to do with this PR, its the generation side let me look into that.

@MIchaelMainer
Copy link
Member

First off, thank you @andreaTP for your time and effort. We appreciate you.

Folks, we shouldn't be merging directly into the branch where we publish from. We should consider creating another branch off of main, create new PR into the branch, merge into that branch, and then the CI/CD can run when you create another PR into main. That way, the community contribution is counted and the CI is run.

@andreaTP
Copy link
Contributor Author

Thanks for chipping in @MIchaelMainer !

Folks, we shouldn't be merging directly into the branch where we publish from.

This is not a common practice, but I have no objection if someone is willing to do the manual work all of the time ...

We should consider creating another branch off of main, create new PR into the branch, merge into that branch, and then the CI/CD can run when you create another PR into main.

This could work, but I expect it to work now as #668 got merged.

At the moment seems like something is broken in Kiota main and the test is correctly failing because of it.

@andreaTP
Copy link
Contributor Author

Just to make it clear, the current CI failure doesn't seem to be related with the changes proposed in this PR.

@andreaTP
Copy link
Contributor Author

andreaTP commented Jun 9, 2023

@baywet sorry for bothering you directly, would you be able to facilitate the progress on this PR?

@andreaTP andreaTP temporarily deployed to build_test June 12, 2023 15:23 — with GitHub Actions Inactive
@baywet
Copy link
Member

baywet commented Jun 12, 2023

@andreaTP it seems our CI is impacted by this microsoft/kiota#2676 which makes it flaky. I just reran the thing for now and it passed.
Thank you for your patience while I was away. We'll need to investigate the generation behaviour, but that's a separate issue we've already captured.
Merging.

@baywet baywet merged commit fd8bfe9 into microsoft:main Jun 12, 2023
@ksdaniel
Copy link

ksdaniel commented Jun 18, 2023

Not sure this is related - but, with Vite and:

  "name": "@microsoft/kiota-abstractions",
  "version": "1.0.0-preview.19"

i get an exception:

Uncaught (in promise) TypeError: Template is not a constructor
    at get URL [as URL] (requestInformation.ts:42:24)
    at FetchRequestAdapter.<anonymous> (fetchRequestAdapter.js:452:45)

Looking at the packages for kiota-abstractions:

  "dependencies": {
    "@opentelemetry/api": "^1.2.0",
    "tinyduration": "^3.2.2",
    "tslib": "^2.3.1",
    "uri-template-lite": "^23.4.0",
    "uuid": "^9.0.0"
  },

we are on the new version of uri-template-lite.

But the code seems to break here:

 const template = new Template(this.urlTemplate);
          const data = {};

aka:

      const template = new (Template as any)(this.urlTemplate);

Got any ideas? This is a rather "blocking" issue.

@baywet
Copy link
Member

baywet commented Jun 18, 2023

Thanks for reporting this. Do you have the same issue if you downgrade the package?

@ksdaniel
Copy link

If I downgrade to

    "@microsoft/kiota-abstractions": "1.0.0-preview.18",
    "@microsoft/kiota-http-fetchlibrary": "^1.0.0-preview.18",
    "@microsoft/kiota-serialization-form": "^1.0.0-preview.8",
    "@microsoft/kiota-serialization-json": "^1.0.0-preview.17",
    "@microsoft/kiota-serialization-text": "^1.0.0-preview.16",

I no longer see the error, mentioned before. (I only downgraded kiota-abstractions)

But now there seems to be a mismatch between kiota-abstractions and kiota-http-fetchlibrary and I get different errors.

Any idea what the working combination might be?

@ksdaniel
Copy link

Just an update - in the end, this combination ran for me:

    "@microsoft/kiota-abstractions": "1.0.0-preview.18",
    "@microsoft/kiota-http-fetchlibrary": "1.0.0-preview.17",
    "@microsoft/kiota-serialization-form": "1.0.0-preview.7",
    "@microsoft/kiota-serialization-json": "1.0.0-preview.16",
    "@microsoft/kiota-serialization-text": "1.0.0-preview.15",

Able to compile and bundle with Vite - the definition mismatches are also gone.

@andreaTP
Copy link
Contributor Author

@ksdaniel would you be able to provide a minimal reproducer so that we can fix the underlying issue?

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

Successfully merging this pull request may close these issues.

uri-template-lite breaks when using a bundler
7 participants