Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

support esnext module type #180

Closed
wants to merge 1 commit into from
Closed

support esnext module type #180

wants to merge 1 commit into from

Conversation

kkalass
Copy link

@kkalass kkalass commented Sep 7, 2018

I have a usecase where I need to use the esnext pkg field as suggested by http://2ality.com/2017/06/pkg-esnext.html .

I am developing web components with the help of skatejs, but without polyfills because I only need to support modern chrome. skatejs exports transpiled sources as module, and original code as esnext.

But the transpiled code does not run on modern browsers because it does not use class inheritance and calls HTMLElement constructor as function instead - which is not legal. So I really need to use the original code as contained in the skatejs package as esnext, controlling myself to which target the entire code packaged by rollup is transpiled.

I would be very happy if you would accept this patch for upstream.

Copy link

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Poke @TrySound @keithamus

if ( !useModule && !useMain && !useJsnext ) {
throw new Error( `At least one of options.module, options.main or options.jsnext must be true` );
if ( !useEsnext && !useModule && !useMain && !useJsnext ) {
throw new Error( `At least one of options.esnext, options.module, options.main or options.jsnext must be true` );
Copy link

Choose a reason for hiding this comment

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

I would sort them from the most to the least used: options.main, options.module, options.jsnext or options.esnext

@@ -25,6 +25,10 @@ export default {
name: 'MyModule',
plugins: [
resolve({
// use "esnext" if possible
Copy link

@ncoden ncoden Sep 26, 2018

Choose a reason for hiding this comment

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

A quick description of the esnext would be useful. package.json entries are confusing enough.

Maybe something like:

  // use "esnext" field for ES6 modules with non-transpiled ES6+ features if possible
  // – see http://2ality.com/2017/06/pkg-esnext.html
  esnext: true,  // Default: false

  // use "module" field for ES6 modules if possible
  // – see http://2ality.com/2017/06/pkg-esnext.html
  module: true,  // Default: true

But you can also take a look at the description I made for the ZURB Foundation documentation

@keithamus
Copy link
Contributor

keithamus commented Sep 27, 2018

In some respects, I feel uneasy about adding "yet another package field". Especially as it is "yet another option"

Perhaps a better strategy is to align closer to webpack's strategy of offering a mainFields option. We can soft-deprecate the jsnext, browser, and module options for now, and move to an implementation where mainFields is checked - in order. This gives users much more flexibility to add nonstandard extra keys without submitting PRs.

@ncoden
Copy link

ncoden commented Sep 27, 2018

@keithamus I agree, but this would require discussions (new API), and takes some times to develop. So maybe we could merge this as it, and refactor the whole "entry points" API in a later pull request.

@keithamus
Copy link
Contributor

If we merge this, then we are just adding API which we know we'll be deprecating. The new API is very straightforward if we just mimick what WebPack does. I'd rather fix it once the right way than introduce code now which we'll only delete in the next PR.

@TrySound
Copy link
Member

I'm all for mainFields

@keithamus keithamus mentioned this pull request Sep 27, 2018
@keithamus
Copy link
Contributor

See #182 which implements mainFields. Would be great to get feedback from all 3 of you.

@kkalass
Copy link
Author

kkalass commented Sep 27, 2018

@keithamus I absolutely agree with generalizing this into mainFields, very good point.

@kkalass kkalass closed this Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants