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

fix: error when using function in webpack output.publicPath #874

Conversation

roland-reed
Copy link

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Fix #873

Breaking Changes

Additional Info

@linux-foundation-easycla
Copy link

CLA Not Signed

1 similar comment
@linux-foundation-easycla
Copy link

CLA Not Signed

@roland-reed roland-reed force-pushed the fix/function-public-path-in-webpack branch from a85b970 to d084255 Compare November 29, 2021 12:59
@alexander-akait
Copy link
Member

It is invalid fix, because hash can be empty

@roland-reed
Copy link
Author

Is this good to be merged? It seems that tests will not start until there's at least one approval

@alexander-akait
Copy link
Member

Please read my answer

@roland-reed roland-reed force-pushed the fix/function-public-path-in-webpack branch from d084255 to c568970 Compare November 30, 2021 05:34
@roland-reed
Copy link
Author

Sorry I read it wrong, I thought it was "valid" 😅. I have changed it to an arbitrary value since hash is not available, which came from the Webpack:

https://github.com/webpack/webpack/blob/main/lib/runtime/PublicPathRuntimeModule.js#L19-L27

@roland-reed
Copy link
Author

Is it okay now? @alexander-akait

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #874 (a89ae76) into master (e8c08a1) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head a89ae76 differs from pull request most recent head a9edf9e. Consider uploading reports for the commit a9edf9e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #874      +/-   ##
==========================================
+ Coverage   91.28%   91.30%   +0.02%     
==========================================
  Files           6        6              
  Lines         792      794       +2     
  Branches      214      215       +1     
==========================================
+ Hits          723      725       +2     
  Misses         63       63              
  Partials        6        6              
Impacted Files Coverage Δ
src/loader.js 89.93% <100.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8c08a1...a9edf9e. Read the comment docs.

@roland-reed roland-reed force-pushed the fix/function-public-path-in-webpack branch from c568970 to a89ae76 Compare December 8, 2021 12:14
@roland-reed
Copy link
Author

Fixed a eslint issue, need another approve to run ci 😄 @alexander-akait

@roland-reed
Copy link
Author

@alexander-akait Lint failed with 0 errors and 3 warnings, do warnings fail the lint? I checked that the 3 warnings are not introduced by this PR

@roland-reed roland-reed force-pushed the fix/function-public-path-in-webpack branch from a89ae76 to a9edf9e Compare December 9, 2021 04:56
@roland-reed
Copy link
Author

@alexander-akait I found it's "lint:prettier" script failed, and I have fixed it. I found that pre-commit hook should format the changed file before commit, but in fact it failed to format the js files, is this behavior expected?
图片

@roland-reed
Copy link
Author

@alexander-akait hello, can this PR be merged? This fix is important for us.

@alexander-akait
Copy link
Member

No, because it is invalid fix

@roland-reed
Copy link
Author

No, because it is invalid fix

And the reason is?

@alexander-akait
Copy link
Member

hash can't be xxxx

@alexander-akait
Copy link
Member

Here fix #881, let's allow webpack to control public path, but yes, you are right, hash is xxxxx, because we are building modules and we don't know hash on this stage

@alexander-akait
Copy link
Member

Anyway thanks for PR, let's close in favor my PR

@roland-reed
Copy link
Author

It's okay, glad that you have fxied it

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.

TypeError: publicPath.replace is not a function when output.publicPath of webpack config is a function
2 participants