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

Add option historyApiFallback #922

Closed
wants to merge 11 commits into from
Closed

Add option historyApiFallback #922

wants to merge 11 commits into from

Conversation

drudrum
Copy link

@drudrum drudrum commented May 30, 2021

This PR contains a:

  • bugfix
  • [ x] new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Adds support of history api (Example: React Router5)
Like this https://webpack.js.org/configuration/dev-server/#devserverhistoryapifallback

Breaking Changes

Additional Info

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Why?

@drudrum
Copy link
Author

drudrum commented Jun 1, 2021

I want use react router5 in my app.
This is not breaking change.

i want add option like as
https://webpack.js.org/configuration/dev-server/#devserverhistoryapifallback

@alexander-akait
Copy link
Member

You can do it on own side, why we need add it here?

@drudrum
Copy link
Author

drudrum commented Jun 1, 2021

Many people want to see compatibility with webpack-dev-server options (i think).
I have not direct and clear access to memfs from my app.
3 rows in library VS 50 rows in each react app using router5

No additional dependencies required. Small useful change.

@alexander-akait
Copy link
Member

Please provide example of the problem, because you can use connect-history-api-fallback on own application side, I don't see any problems in our bug trackers from other developers

@drudrum
Copy link
Author

drudrum commented Jun 1, 2021

2021-06-01_17-34

@alexander-akait
Copy link
Member

Can you create simple git repo? I think I understand the problem, but I would like to check this before merge? Thanks

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

ah, you want to read index.html from memory fs

We should add docs and tests on this option, please fix it and I will merge

@drudrum
Copy link
Author

drudrum commented Jun 1, 2021

Readme at 59382db

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (f028875) 97.34% compared to head (df8d94d) 97.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #922      +/-   ##
==========================================
+ Coverage   97.34%   97.38%   +0.03%     
==========================================
  Files           9        9              
  Lines         377      382       +5     
  Branches      112      116       +4     
==========================================
+ Hits          367      372       +5     
  Misses          9        9              
  Partials        1        1              
Impacted Files Coverage Δ
src/index.js 97.91% <ø> (ø)
src/utils/getFilenameFromUrl.js 98.38% <100.00%> (+0.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@drudrum
Copy link
Author

drudrum commented Jun 2, 2021

How i can fix it?
It is a older dependency

@alexander-akait
Copy link
Member

Please rebase, should work

@drudrum
Copy link
Author

drudrum commented Jun 2, 2021

Workflow dislike me ))
How i can fix this problem?

@drudrum drudrum closed this Jun 3, 2021
@drudrum drudrum reopened this Dec 29, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 29, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 29, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

type MultiCompiler = import('webpack').MultiCompiler;
type Compilation = import('webpack').Compilation;
type IncomingMessage = import('../index.js').IncomingMessage;
type ServerResponse = import('../index.js').ServerResponse;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we need to run prettier on all types files, can you do it?

Copy link
Author

Choose a reason for hiding this comment

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

Yes of course

@alexander-akait
Copy link
Member

@drudrum Friendly ping

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 9, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@drudrum
Copy link
Author

drudrum commented Mar 9, 2023

@alexander-akait Sorry. We should finish this old PR :-)

@drudrum
Copy link
Author

drudrum commented Mar 9, 2023

@alexander-akait Can you find approvers for this PR?

@drudrum drudrum requested a review from alexander-akait March 10, 2023 14:04
!context.outputFileSystem.existsSync(filename) &&
options.historyApiFallback
) {
filename = path.join(outputPath);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need path.join here, it doesn't have sense

Copy link
Author

Choose a reason for hiding this comment

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

I will check. Look like mistake. May be we need change it to path.resolve

Copy link
Author

Choose a reason for hiding this comment

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

Now this code looks like this option is not necessarily at all.

Copy link
Author

Choose a reason for hiding this comment

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

Everything should work without this option.
I am confused.
I feel that i need cancel this PR.

@drudrum drudrum closed this Mar 12, 2023
@drudrum
Copy link
Author

drudrum commented Mar 12, 2023

This is duplicated PR. We have the same #930

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.

2 participants