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

Allow [format] pattern for ID generation #2387

Merged
merged 2 commits into from
Aug 11, 2018

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Aug 8, 2018

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

Well, there is possible (but HIGHLY unlikely) breakage. If somebody has uses [format] in their filenames.

Please Describe Your Changes

They are super simple, I've just added support for [format] in file names patterns.

@guybedford
Copy link
Contributor

Sure, I'd be happy to include this.

We should just check though that it does work well with unsanitized format inputs - that is we allow format: 'es', format: 'esm' and format: 'es6' so we should make sure the output format is consistent for those cases - esm perhaps.

@Andarist
Copy link
Member Author

Andarist commented Aug 8, 2018

Oh, to clarify things for me - you would like to ensure [format] can be mapped to es & es6 because now it is going to get mapped to esm as by the time [format] is resolved the outputOptions.format is already normalized?

With v1 coming soon-ish (right? 😄) is it worth adding extra (& temporary) code to support this?

@guybedford
Copy link
Contributor

@Andarist the opposite - if providing format: es it should still output [format] as 'esm' I think?

@Andarist
Copy link
Member Author

Andarist commented Aug 8, 2018

So this is actually other way around 😄 esm gets normalized to es here and that normalized output.format reached the pattern replacement code, so no matter what you pass in originally you will get es as [format].

I think you want to normalize this other way around? And unify naming to esm now, right? This is kinda out of scope of this PR. I can prepare a followup PR for this.

@guybedford
Copy link
Contributor

Ah I see, best to leave format internally to es now. Then perhaps we should just pre-emtively add the renormalization to esm for this output specifically?

@guybedford
Copy link
Contributor

Or do you think es is better for this use case?

@Andarist
Copy link
Member Author

Andarist commented Aug 8, 2018

Personally I would avoid renormalization, seems weird to normalize it one way first & renormalize it "back" later. I can do it if you feel it's the right thing to do though. Is there any problem with switching to esm internally?

Or do you think es is better for this use case?

Kinda don't care that much about it 😄 I'm fine with any decision you can make about this. The whole purpose of this (for me) is to reuse dir for both cjs & esm, but for that I need to distinguish the output files (can ofc use current entryFileNames with hardcoded format in it, but thought that this would be useful for others to include this in the rollup itself)

@TrySound
Copy link
Member

TrySound commented Aug 8, 2018

I'm all for consistent esm name

@shellscape
Copy link
Contributor

Side Tangent: Just a little Rollup history question. Is there a reason we're using square brackets for filename "templates" instead of one of the other usual template-indicator characters, like curly braces?

@guybedford
Copy link
Contributor

@Andarist we just need to still ensure that the observable format in options for plugins matches what we have right now - es as plugins may be doing checks. So it is still a breaking change to switch to esm for options.format I think, so best to wait till 1.0 there.

Let's go the convoluted route for now I think :)

Would be great to get this updated and merged.

@Andarist
Copy link
Member Author

Done

@guybedford
Copy link
Contributor

Thanks! The test file just needs to be renamed to use the esm name.

@Andarist
Copy link
Member Author

Damn, forgot about file names references in file contents 😅 I think it should be OK now.

@guybedford
Copy link
Contributor

Great thanks, just needs one more approval to merge.

@guybedford guybedford merged commit 4e67254 into rollup:master Aug 11, 2018
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

4 participants