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

feat: add invalidate endpoint #2493

Merged
merged 3 commits into from
Apr 15, 2020

Conversation

EslamHiko
Copy link
Member

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

yes

complete: #1546

Motivation / Use-Case

Breaking Changes

no

Additional Info

@@ -64,6 +64,10 @@ describe('routes util', () => {
});
});

it('GET request to invalidate endpoint', (done) => {
req.get('/invalidate').expect(200, done);
});
Copy link
Member

Choose a reason for hiding this comment

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

Will be great to test what we really do invalidate - get stats before do request and do get stats again

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't detect any changes in server.middleware

Copy link
Member Author

Choose a reason for hiding this comment

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

and server.middleware.context.stats is always undefined

Copy link
Member

Choose a reason for hiding this comment

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

Strange, should be no

Copy link
Member Author

@EslamHiko EslamHiko Apr 1, 2020

Choose a reason for hiding this comment

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

I've thought about an Idea maybe we can create an option invalidateCallback and we create a function that creates a file then delete after confirming that the file exists.
@evilebottnawi what do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

Bad idea, maybe we need fix something in tests, need debug

@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #2493 into master will decrease coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2493      +/-   ##
==========================================
- Coverage   93.74%   93.58%   -0.17%     
==========================================
  Files          34       34              
  Lines        1327     1325       -2     
  Branches      381      379       -2     
==========================================
- Hits         1244     1240       -4     
- Misses         81       83       +2     
  Partials        2        2              
Impacted Files Coverage Δ
lib/Server.js 96.76% <100.00%> (-0.03%) ⬇️
lib/utils/routes.js 93.47% <100.00%> (+0.97%) ⬆️
client-src/clients/SockJSClient.js 57.50% <0.00%> (-2.50%) ⬇️
client-src/clients/WebsocketClient.js 55.00% <0.00%> (-2.50%) ⬇️
lib/servers/WebsocketServer.js 93.93% <0.00%> (-0.18%) ⬇️
lib/utils/runOpen.js 100.00% <0.00%> (ø)

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 0e9bffb...37a2705. Read the comment docs.

@@ -118,7 +118,7 @@ class Server {
this.setupDevMiddleware();

// set express routes
routes(this.app, this.middleware, this.options);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@hiroppy To pass the server, so I can call server.invalidate()

Copy link
Member

@hiroppy hiroppy Apr 3, 2020

Choose a reason for hiding this comment

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

I'm -1 on this change because if you pass this, we can't understand what variables are used by this function clearly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hiroppy I tried (this.app, this.middleware, this.options, this.invalidate) and called invalidate() only but this resulted in 500 "Internal Server Error" so what about changing this into server for better reading ex:

const server = this;
routes(server);

Copy link
Member Author

Choose a reason for hiding this comment

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

one thing that worked was moving

invalidate(callback) {
    if (this.middleware) {
      this.middleware.invalidate(callback);
    }
  }

to routes.js and make it only accessable via the api call, but that would require adding an option invalidateCallBack so we can pass it ex:

    this.invalidateCallBack = this.options.invalidateCallBack;
    // set express routes
    routes(this.app, this.middleware, this.options, this.invalidateCallBack);

and in routes.js

function routes(app, middleware, options, invalidateCallBack) {
  app.get('/invalidate', (_req, res) => {
    if(middleware)
      middleware.invalidate(invalidateCallBack)
    res.end();
  });
}

@@ -118,7 +118,7 @@ class Server {
this.setupDevMiddleware();

// set express routes
routes(this.app, this.middleware, this.options);
Copy link
Member

@hiroppy hiroppy Apr 3, 2020

Choose a reason for hiding this comment

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

I'm -1 on this change because if you pass this, we can't understand what variables are used by this function clearly.

@alexander-akait
Copy link
Member

@hiroppy I think it is normal to pass this in some cases, like webpack pass compiler and compilation to hooks and utils functions

@hiroppy
Copy link
Member

hiroppy commented Apr 3, 2020

I got it but it's hard to read for me like a black box.

@alexander-akait
Copy link
Member

@hiroppy I think we can do refactor for utils and code for next branch, we have many bad places that we can improve, maybe using destructuring is good idea in many places

@hiroppy
Copy link
Member

hiroppy commented Apr 3, 2020

@evilebottnawi Totally agree with you. I'll improve these codes on the next branch.

@alexander-akait
Copy link
Member

@hiroppy So i think we can merge it, we should fix the problem #1720 (we can't remove inline while not fix it + it will good for next) and do release

@alexander-akait
Copy link
Member

/cc @hiroppy

@@ -30,6 +34,11 @@ function routes(app, middleware, options) {
createReadStream(join(clientBasePath, 'live.html')).pipe(res);
});

app.get('/invalidate', (_req, res) => {
Copy link
Member

@sokra sokra May 5, 2020

Choose a reason for hiding this comment

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

This should better be prefixed with webpack-dev-server maybe /webpack-dev-server/invalidate. Otherwise /invalidate is unusable for the user and apps using /invalidate in their router break.

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.

4 participants