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 in-memory sourcemap filename #2161

Merged
merged 2 commits into from
Apr 28, 2018
Merged

Fix in-memory sourcemap filename #2161

merged 2 commits into from
Apr 28, 2018

Conversation

guybedford
Copy link
Contributor

This fixes sourcemap file setting for in-memory builds, in order to fix a test regression of the CommonJS plugin. I've also brought that test into the misc tests of Rollup.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Great fix.

Any objections to rebasing this to the latest released commit, i.e. commit d21b935? That way I can easily release this as a patch release without the need to release other merged functionality.

@lukastaegert
Copy link
Member

Will try to review the other fixes soon as well, thanks for digging into this! I will try to do separate bug fix releases off the branches if they can be rebased against the latest release (but I can try to do this myself of course).

@guybedford
Copy link
Contributor Author

Ah, I didn't realise the releases had diverged from master. I'd suggest we reconcile that first as I do think it is important to keep master and releases in sync as it makes debugging and bisecting workflows much easier having a linear history, at least within major ranges.

guybedford added a commit to rollup/rollup-plugin-commonjs that referenced this pull request Apr 27, 2018
@lukastaegert
Copy link
Member

That means I do another feature release without basically any content except for the ESM alias and some minor other changes?

I do not see an issue with releasing from a branch (as I did for 0.58.1 and 0.58.2) as it gives us much more flexibility to merge bigger PRs into master without releasing them right away. I would immediately merge into master after the release.

@lukastaegert
Copy link
Member

Also since the history is fully preserved, bisecting should not be any more difficult.

@lukastaegert
Copy link
Member

I.e. this is about giving as flexibility and quicker response time and fixing regressions within a patch of the release they were introduced without introducing new functionality that may again introduce new bugs.

Otherwise I would not want to release this and try to review all the other fixes first which, at my current review speed, feels more like next week, so that this is a proper release and people do not ask "why was this and that not included!?"

@guybedford
Copy link
Contributor Author

Definitely understood release branches allow a quicker process if there is too much to check on master. This is what NodeJS does as well, but there are a few things just to consider from a process perspective with this stuff.

It would also help to hear what on the master branch isn't release and for what reasons we're holding them back? Do you see this as a temporary thing for now, or is this the sort release process you'd like to set up for Rollup to allow fast-merges on master? Worth having these discussions definitely.

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.

None yet

2 participants