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

Jlc/parser to know path #568

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Conversation

johanseto
Copy link
Contributor

@johanseto johanseto commented Sep 26, 2023

Description:

This a response to the suggestion of the fix due error with PUBLIC_PATH. Because webpack complete URLS and not only paths. For eg for cases like CDN builts.
The details are more explained in the following PR due is recommended to have compatibility and support of webpack.
openedx/frontend-build#398 (comment)

Commit purposes

feat: add parser url function to utils
feat: add getPath function for PUBLIC_PATH

This adds a function to get a path from a string URL from utils.
Also, the function is implemented to the history module. In order to match with URLS as webpack allow.(CDN).
Tests were developed for different shapes of string urls with path.
A basic test is also added for the base case in the history variable for the initialize
with a base case of PUBLIC_PATH=/.

Merge checklist:

  • Consider running your code modifications in the included example app within frontend-platform. This can be done by running npm start and opening http://localhost:8080.
  • Consider testing your code modifications in another local micro-frontend using local aliases configured via the module.config.js file in frontend-build.
  • Verify your commit title/body conforms to the conventional commits format (e.g., fix, feat) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.

Post merge:

  • After the build finishes for the merged commit, verify the new release has been pushed to NPM.

This add function to get path from a string url from utils.
Also the function is implemented to the history module. In order to match with URLS as webpack allow.(CDN).
Test were developed for different shaped of string urls with path.
A basic test is also added for the base case in the history variable for the initialize
with base case of PUBLIC_PATH=`/`.
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 26, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @johanv26! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4302286) 83.08% compared to head (f842f98) 83.17%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #568      +/-   ##
==========================================
+ Coverage   83.08%   83.17%   +0.09%     
==========================================
  Files          40       40              
  Lines        1064     1070       +6     
  Branches      195      195              
==========================================
+ Hits          884      890       +6     
  Misses        168      168              
  Partials       12       12              
Files Coverage Δ
src/initialize.js 98.78% <ø> (ø)
src/utils.js 97.29% <100.00%> (+0.52%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for the comprehensive tests!

@arbrandes arbrandes merged commit 93ea5b2 into openedx:master Sep 27, 2023
6 checks passed
@openedx-webhooks
Copy link

@johanv26 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

dcoa pushed a commit to eduNEXT/frontend-platform that referenced this pull request Nov 8, 2023
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
MoisesGSalas pushed a commit to eduNEXT/frontend-platform that referenced this pull request Dec 11, 2023
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
MoisesGSalas pushed a commit to eduNEXT/frontend-platform that referenced this pull request Dec 11, 2023
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
dcoa added a commit to eduNEXT/frontend-platform that referenced this pull request Jan 3, 2024
ztraboo added a commit to CUCWD/frontend-platform that referenced this pull request Apr 14, 2024
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
ztraboo added a commit to CUCWD/frontend-platform that referenced this pull request Apr 14, 2024
Handle absolute URIs in PUBLIC_PATH (openedx openedx#568) for maple release.
dcoa pushed a commit to eduNEXT/frontend-platform that referenced this pull request Apr 16, 2024
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
bra-i-am pushed a commit to eduNEXT/frontend-platform that referenced this pull request Apr 30, 2024
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
bra-i-am pushed a commit to eduNEXT/frontend-platform that referenced this pull request Apr 30, 2024
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
bra-i-am pushed a commit to eduNEXT/frontend-platform that referenced this pull request May 2, 2024
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
becdavid pushed a commit to CUCWD/frontend-platform that referenced this pull request May 8, 2024
Webpack allows `publicPath` to be an absolute URI (https://webpack.js.org/configuration/output/#outputpublicpath), in order to support deployments to a CDN.  However, the browser history module expects a relative path for `basename` but uses the same environment variable (`PUBLIC_PATH`) to configure it.  Since the path is always expected to be be suffixed to the absolute URI, we avoid introducing another configuration variable (thus maintaining full backwards compatibility) by extracting it at initialization time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants