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(regression): Webpack not working on Windows with new export registry #3855

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

dsevillamartin
Copy link
Member

Caused by #3842.

Changes proposed in this pull request:

  • Use path.relative for module path
  • Replace system path separators with /

Initially, since .replace() was hardcoding /, the entire absolute path was being added to flarum.reg.add().
Then, building still errored because it was using \ as path separators, and the following characters weren't valid escape sequences.

ERROR in ./src/common/utils/styleSelectedText.ts 354:32
Module parse failed: Bad character escape sequence (354:32)
File was processed with these loaders:
 * ../../../js-packages/webpack-config/src/autoExportLoader.cjs
 * ../../../node_modules/babel-loader/lib/index.js
You may need an additional loader to handle the result of these loaders.
|   };
| }
> flarum.reg.add('core', 'common\utils\styleSelectedText', styleSelectedText)

Tried to use path.posix, but it can't normalize win32 paths into unix. There's a package for that (slash), but it basically just replaces \ with / - I don't think this implementation should cause any issues.

QA
Build JS. Dist remains the exact same.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@dsevillamartin dsevillamartin requested a review from a team as a code owner July 13, 2023 16:27
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Thanks!

@SychO9 SychO9 changed the title Fix Webpack not working on Windows with new export registry fix(regression): Webpack not working on Windows with new export registry Jul 27, 2023
@SychO9 SychO9 merged commit c80220a into 2.x Jul 27, 2023
9 of 19 checks passed
@SychO9 SychO9 deleted the ds/fix-webpack-registry-windows branch July 27, 2023 10:22
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.

2 participants