Skip to content

Enable watcher in code-splitting mode - #2035

Merged
lukastaegert merged 3 commits into
rollup:masterfrom
anandthakker:watch-code-splitting
Mar 12, 2018
Merged

Enable watcher in code-splitting mode#2035
lukastaegert merged 3 commits into
rollup:masterfrom
anandthakker:watch-code-splitting

Conversation

@anandthakker

@anandthakker anandthakker commented Mar 5, 2018

Copy link
Copy Markdown
Contributor

Closes #1950

  • Update typing for rollup.rollup() to reflect code-splitting output type.
  • Update watcher to handle code-splitting output.
  • Add tests for watcher in code-splitting mode.
  • Lint

@anandthakker
anandthakker force-pushed the watch-code-splitting branch from 3052d90 to 548081b Compare March 5, 2018 21:28
@anandthakker

Copy link
Copy Markdown
Contributor Author

@guybedford @lukastaegert I believe this should be ready for a 👀

@anandthakker anandthakker changed the title Enable watch mode in code-splitting mode Enable watcher in code-splitting mode Mar 5, 2018
@lukastaegert lukastaegert added this to the 0.57.0 milestone Mar 6, 2018

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Amazing work, thanks so much for working on this, these sorts of contributions remind me why open source is worthwhile :)

Comment thread src/watch/index.ts Outdated
}
} else {
const chunk = (<OutputChunk>result);
this.cache = { chunks: { [this.inputOptions.entry]: chunk } };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the cache consists of chunks in both cases, do we still need the this.cache.modules check in https://github.com/rollup/rollup/pull/2035/files#diff-7c1e6304262c178e7fe268b62060e26bR85 ?

@anandthakker anandthakker Mar 6, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, good question. I should have explained this in the code.

Task#cache is typed as {chunks: { [name]: { modules } } } even in the single-bundle case just to keep this code slightly simpler.

However, for the type of InputOptions.cache, which is part of the public JS API, I went with { modules } | {chunks: { [name]: { modules } } } so that this wouldn't be a breaking change (and because I think the existing cache option, wherein a developer can simply pass the previous bundle result back in as cache:, seems to make sense).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Task#cache is typed as {chunks: { [name]: { modules } } } even in the single-bundle case just to keep this code slightly simpler.

On second thought, I think it's probably clearer and easier to understand in the future if we just keep Task#cache typed identically to InputOptions#cache. I'll push that change.

@anandthakker

Copy link
Copy Markdown
Contributor Author

@guybedford ah, whoops I didn't realize I'd gotten behind master. I'll rebase & resolve now.

@anandthakker
anandthakker force-pushed the watch-code-splitting branch from 3403d98 to 07da0b0 Compare March 6, 2018 14:08
@anandthakker

Copy link
Copy Markdown
Contributor Author

Updated Task#cache typing and rebased onto master

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really works like a charm! I have no concerns with merging this.

@lukastaegert

Copy link
Copy Markdown
Member

On another note: I think for the future we should work towards re-implementing the non-code-splitting case to be handled as a single chunk code-splitting case to get rid of all the duplicity we currently have in some places. Once we do that, we will be able to again have only one type of cache.

@anandthakker

Copy link
Copy Markdown
Contributor Author

@guybedford @lukastaegert anything else you need here before this can be merged?

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.

3 participants