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

Refactor webpack configuration to improve library exports #1565

Conversation

KD1712
Copy link
Collaborator

@KD1712 KD1712 commented May 22, 2024

Changed the webpack configuration to specify library as 'UpgradeClient' and libraryExport as 'default' to improve library exports. This ensures that users can directly access the UpgradeClient class without needing to access it through additional objects. Also updated the globalObject to 'this' for better compatibility across environments.

@KD1712
Copy link
Collaborator Author

KD1712 commented May 23, 2024

@bcb37 Here 'output.library' is removed from the webpack config to get direct access to the modules. Should we keep it this way for easier module access, or do we keep the library name for namespacing?

@ppratikcr7 ppratikcr7 linked an issue May 24, 2024 that may be closed by this pull request
@VivekFitkariwala
Copy link
Collaborator

@KD1712 can you check the PR? There are some extra change.

@KD1712 KD1712 force-pushed the code-improvement/import-client-without-webpack-150 branch from 939afda to 22a833e Compare June 11, 2024 05:51
@KD1712 KD1712 force-pushed the code-improvement/import-client-without-webpack-150 branch 2 times, most recently from 22a833e to c4bd47e Compare June 11, 2024 07:04
@@ -34,8 +34,10 @@ const browser = {
output: {
filename: 'index.js',
path: path.resolve(__dirname, 'dist/browser'),
libraryExport: 'default',
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are four variations where the same change needs to be done. Can you refactor the code and extract the common code?

@KD1712 KD1712 requested a review from VivekFitkariwala June 19, 2024 09:56
@danoswaltCL
Copy link
Collaborator

@zackcl can you verify this is the change that was needed in demo app for vanilla js when you get back?

@danoswaltCL danoswaltCL requested a review from zackcl July 22, 2024 14:47
@danoswaltCL
Copy link
Collaborator

hey @zackcl this is related to a very old issue that you opened to help (#150), is this what you were looking for?

@zackcl
Copy link
Collaborator

zackcl commented Jul 22, 2024

@danoswaltCL I will check this now.

@zackcl
Copy link
Collaborator

zackcl commented Jul 22, 2024

I tried the client library on this branch, and I was able to create the UpgradeClient class directly with const upClient = new UpgradeClient(userId, hostUrl, context);, which is what I expected.

However, while the clientlibs/js/dist/browser/index.js script works fine, the clientlibs/js/dist/browser-lite/index.js script does not.

When using the browser-lite version, I get the error: Please provide a valid httpClient, or use the default version of the library with our default httpClient.

You can see the code I wrote for testing the client library here: https://jsfiddle.net/my93L5bg/.

@ppratikcr7
Copy link
Collaborator

ppratikcr7 commented Aug 20, 2024

I tried the client library on this branch, and I was able to create the UpgradeClient class directly with const upClient = new UpgradeClient(userId, hostUrl, context);, which is what I expected.

However, while the clientlibs/js/dist/browser/index.js script works fine, the clientlibs/js/dist/browser-lite/index.js script does not.

When using the browser-lite version, I get the error: Please provide a valid httpClient, or use the default version of the library with our default httpClient.

You can see the code I wrote for testing the client library here: https://jsfiddle.net/my93L5bg/.

Yeah, I do get the same error. For dist/browser on dev branch build, I had to update the apiVersion to v5 to make it work and get assignment. But doing the same change with index.js for dist/browser-lite still gives the above error.

Screenshot 2024-08-20 at 6 39 54 PM

@ppratikcr7
Copy link
Collaborator

ppratikcr7 commented Aug 21, 2024

@zackcl I have updated the fiddle, we need to provide the httpClient object as options in the UpgradeClient so that it returns the assignment for browser-lite distribution. For browser distribution you dont need to pass this httpClient object.

@zackcl
Copy link
Collaborator

zackcl commented Aug 21, 2024

@ppratikcr7 Thanks, so should we expect users to add this code when using "browser-lite"? I'm not sure if this is worth including in our documentation in case they use "browser-lite". If it's difficult to support "browser-lite", maybe we can just document that the lite version is not supported for JavaScript apps at the moment, and fix this later.

let httpClient = {
      doGet: (path, options) => {
        return fetch(path, options).
        then(response => response.json());
      },
      doPost: (path, body, options) => {
        body = JSON.stringify(body);
        return fetch(path, { method: "POST", body, ...options }).
        then(response => response.json());
      },
      doPatch: (path, body, options) => {
        body = JSON.stringify(body);
        return fetch(path, { method: "PATCH", body, ...options }).
        then(response => response.json());
      },
    };
    const options = { httpClient : httpClient }
    const upClient = new UpgradeClient(userId, hostUrl, context, options);
    ```

@zackcl
Copy link
Collaborator

zackcl commented Aug 21, 2024

I will review the updated fiddle soon.

@zackcl
Copy link
Collaborator

zackcl commented Aug 23, 2024

@ppratikcr7 Your fiddle worked fine with the custom HTTP client. I've just created a PR (#1862) suggesting the use of a default HTTP client for the lite version as well. Please feel free to check it out.

By the way, I had to change the version in clientlibs/js/package.json from "6.0.0" to "5.2.0" in the current dev branch; otherwise, I got an error. I'm not sure if this is because we don't have v6 endpoints yet.

zackcl
zackcl previously approved these changes Aug 23, 2024
@danoswaltCL
Copy link
Collaborator

@ppratikcr7 can you take over this PR and look at @zackcl 's comment, then we can get this in for 6.0

@KD1712 KD1712 dismissed stale reviews from zackcl and VivekFitkariwala via f9a779f September 6, 2024 05:33
@ppratikcr7
Copy link
Collaborator

@ppratikcr7 can you take over this PR and look at @zackcl 's comment, then we can get this in for 6.0

@zackcl Can you reapprove this and merge?

@zackcl zackcl merged commit 279d904 into CarnegieLearningWeb:dev Sep 6, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing the UpGrade client library (JS) without Webpack
5 participants