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

Leave it up to resolveId to normalize the entry path. #835

Merged
merged 3 commits into from
Aug 7, 2016

Conversation

lemmabit
Copy link
Contributor

@lemmabit lemmabit commented Aug 7, 2016

Normalizing the entry ID before passing it to any plugins is redundant, because if it's a path, it will be normalized in resolveId like any other ID. If it isn't a path, normalizing it before passing it to resolveId may mangle it in a surprising way, such as by replacing backslashes with forward slashes.

@TrySound
Copy link
Member

TrySound commented Aug 7, 2016

@Permutatrix Surprising test? :)

@lemmabit
Copy link
Contributor Author

lemmabit commented Aug 7, 2016

You mean like this? That test fails without this PR and succeeds with it. Or do you want me to add a new test to the PR?

@TrySound
Copy link
Member

TrySound commented Aug 7, 2016

Yep, in the PR. We should prevent accidental adding it again.

@lemmabit
Copy link
Contributor Author

lemmabit commented Aug 7, 2016

Alright, there you go. Are tests a requirement for every PR?

@TrySound
Copy link
Member

TrySound commented Aug 7, 2016

@Permutatrix If it modifies functionality, yes. Can this PR break something? I mean should I publish middle version?

@lemmabit
Copy link
Contributor Author

lemmabit commented Aug 7, 2016

@TrySound It's a long shot, but there could be a very poorly configured setup out there somewhere that looks something like this:

// rollup.config.js
import hypothetical from 'rollup-plugin-hypothetical';

export default {
  entry: 'x\\y',
  plugins: [hypothetical({
    files: {
      'x/y': `
        console.log("What a contrived example!");
      `
    },
    leaveIdsAlone: true
  })]
};

Any change that fixes behavior has the potential to break things that relied on the incorrect behavior, though. I wouldn't bump the minor version number for this myself, but it's obviously up to you.

@TrySound TrySound merged commit 9500fd6 into rollup:master Aug 7, 2016
@TrySound
Copy link
Member

TrySound commented Aug 7, 2016

Thanks.

@lemmabit lemmabit deleted the plain-entry-path branch August 7, 2016 09:29
@TrySound
Copy link
Member

TrySound commented Aug 7, 2016

0.34.7

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