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.

Sign up for GitHub

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
Merged

feat: add invalidate endpoint #2493

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da0f358
feat: add invalidate endpoint
EslamHiko Apr 1, 2020
d70af03
Merge branch 'master' into invalidate-endpoint
EslamHiko Apr 2, 2020
37a2705
Merge branch 'master' into invalidate-endpoint
hiroppy Apr 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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();
  });
}

routes(this);

// Keep track of websocket proxies for external websocket upgrade.
this.websocketProxies = [];
Expand Down
11 changes: 10 additions & 1 deletion lib/utils/routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ const { join } = require('path');

const clientBasePath = join(__dirname, '..', '..', 'client');

function routes(app, middleware, options) {
function routes(server) {
const app = server.app;
const middleware = server.middleware;
const options = server.options;

app.get('/__webpack_dev_server__/live.bundle.js', (req, res) => {
res.setHeader('Content-Type', 'application/javascript');

Expand All @@ -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 •
edited
Loading

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.

server.invalidate();
res.end();
});

app.get('/webpack-dev-server', (req, res) => {
res.setHeader('Content-Type', 'text/html');

Expand Down
4 changes: 4 additions & 0 deletions test/server/utils/routes.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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


it('should handles GET request to live html', (done) => {
req.get('/webpack-dev-server/').then(({ res }) => {
expect(res.headers['content-type']).toEqual('text/html');
Expand Down