Skip to content

Fix in-memory sourcemap filename - #2161

Merged
lukastaegert merged 2 commits into
masterfrom
sourcemaps-name-fix
Apr 28, 2018
Merged

Fix in-memory sourcemap filename#2161
lukastaegert merged 2 commits into
masterfrom
sourcemaps-name-fix

Conversation

@guybedford

Copy link
Copy Markdown
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.

@guybedford
guybedford force-pushed the sourcemaps-name-fix branch from 3004507 to 4aba7fe Compare April 26, 2018 18:30

@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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@lukastaegert

Copy link
Copy Markdown
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
Copy Markdown
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.

2 participants