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

add jspm configuration #7

Merged
merged 1 commit into from
Dec 9, 2015
Merged

add jspm configuration #7

merged 1 commit into from
Dec 9, 2015

Conversation

guybedford
Copy link
Contributor

This PR includes the necessary jspm configuration for this package to work in Node with the coming jspm 0.17 release, allowing this single package to provide both client and server support.

I completely understand if you'd rather not support this here, but it will simplify user configuration locally having it in one place. There is no maintenance burden either as I would continue to maintain this personally.

Just let me know if you have any questions at all, and thanks for considering.

@balupton
Copy link
Member

Hey @guybedford,

Thanks. We are already working towards adding jspm support for all our packages, so this will help with it.

I'm not familiar with the 0.17 release. Our other modules all more or less do this:
https://github.com/bevry/eachr/blob/c1c5c7cae088a311947330272bfc7f79add3ff37/package.json#L74-L78

As recommended by step 4 of our estnextguardian package:
https://github.com/bevry/esnextguardian#usage

Is there something special about the "map" stuff that is more beneficial than just pointing "main" somewhere? Keen to find out.

Cheers!

@guybedford
Copy link
Contributor Author

@balupton great to hear you're already looking at providing jspm support! The jspm-specific main approach is exactly the way to handle it.

The 0.17 release will likely only be fully released in Jan, but the goal is to get an alpha out in December.

The specific thing going on here is about branching code between Node environments and non-Node environments so that this implementation can be replaced with the Node core implementations when running in Node via this config.

To explain the jspm 0.17 changes, if you are interested, the package.json configuration spec is becoming exactly the SystemJS package configuration (https://github.com/systemjs/systemjs/blob/master/docs/config-api.md#packages). Then we're using conditional package map here which is still undocumented until we've got conditional build branching completely stable.

@balupton
Copy link
Member

Hmmmmm.... A few questions then.

Why this:

{
    "jspm": {
        "map": {
            "./index.js": {
                "node": "@node/domain"
            }
        }
    }
}

Over this:

{
    "jspm": {
        "main": "./index.js"
    }
}

What is the difference? Why is "map" appropriate for this project over just "main", as done in bevry/eachr?

Finally, considering that v0.17 is coming later, shouldn't we do both? Is this preferred?

{
    "jspm": {
        "main": "./indexjs",
        "map": {
            "./index.js": {
                "node": "@node/domain"
            }
        }
    }
}

Cheers!

@guybedford
Copy link
Contributor Author

Sure! The idea is that a user can do jspm install domain=npm:domain-browser and have that package work equally well between client and server. I could have created a wrapper package, and did actually do this previously, but the dependency relation is a lot simpler if we put the branching logic here into this package itself.

So the config just says use the Node core domain module when in Node instead of the index.js file of this package.

We could include the main in the jspm config as well certainly, but by default jspm will follow the Node convention since this package is on npm, and use index.js as the main anyway.

Does that answer your question?

@balupton
Copy link
Member

Perfect. Makes absolute sense!

We could include the main in the jspm wrapper as well certainly, but by default jspm will follow the Node convention since this package is on npm, and use index.js as the main anyway.

So if my learning is correct, the only reason we have "main" in eachr is because there are different editions of the file for node (prefer modern/esnext, fallback to legacy/es5), browserify (use legacy), and jspm (use modern):

{
  "main": "./esnextguardian.js",
  "browser": "./es5/lib/eachr.js",
  "jspm": {
    "main": "./esnext/lib/eachr.js"
  }
}

But as domain-browser has only one edition, no need for any of that bevry/eachr style configuration.

@guybedford
Copy link
Contributor Author

Yes exactly.

Just taking a look now though, I'm not sure if the jspm main will work out of the box in all cases since for example the file at https://github.com/bevry/eachr/blob/master/esnext/lib/eachr.js is both an ES module and a CommonJS module, so that jspm won't be able to load it.

Unfortunately this means that jspm would likely have to load the ES5 version rather, so it may be worth leaving out the jspm-specific config in this case.

If it was import typeChecker from 'typechecker' at the top, that would work in theory, but even then only when also adding jspmNodeConversion: false to the jspm config, pending jspm 0.17 which will no longer need this.

@balupton
Copy link
Member

Just taking a look now though, I'm not sure if the jspm main will work out of the box in all cases since for example the file at https://github.com/bevry/eachr/blob/master/esnext/lib/eachr.js is both an ES module and a CommonJS module, so that jspm won't be able to load it.

Wow... the realisation that esnext modules don't work in node actually defeats the purpose of esnextguardian for all modules that use esnext modules instead of commonjs require and exports.

Will likely move all packages back to commonjs exports until node gains official support, as well as adding a note to esnextguardian for users of it to avoid esnext modules if they are wanting the benefits:

/cc nodejs/help#31


Considering a move back to commonjs exports, that configuration as before makes sense?

@guybedford
Copy link
Contributor Author

Yes certainly that makes sense. You could also potentially provide a workflow to write with modules and transpile two versions - one that just does the module transpilation, and one that does all ES5 transpilations.

Unfortunately again jspm currently doesn't support ES feature transpilation for non-modules yet (pending this coming release!), so would still need to use the ES5 version in this case until 0.17 is out.

@balupton
Copy link
Member

balupton commented Dec 1, 2015

Unfortunately again jspm currently doesn't support ES feature transpilation for non-modules yet (pending this coming release!)

Is there a link to this so I can read up on it?

As I thought it already did, hence what the transpiler docs at https://github.com/systemjs/systemjs#basic-use and https://github.com/systemjs/systemjs/blob/master/docs/config-api.md#transpiler are for...

Or rather what points these points on http://jspm.io are for:

  • For development, load modules as separate files with ES6 and plugins compiled in the browser.
  • For production (or development too), optimize into a bundle, layered bundles or a self-executing bundle with a single command.

Or rather what "Supports Traceur, Babel and TypeScript for compiling ES6 modules and syntax into ES5 in the browser with source map support." is for at https://github.com/ModuleLoader/es6-module-loader

@guybedford
Copy link
Contributor Author

It is due to the original architecture of SystemJS, where the change is discussed here - systemjs/systemjs#579.

The point is that the module format of the file is coupled to the language of the file in SystemJS currently. While in reality I can have an ES6 file which is CommonJS, which is exactly the assumption that is breaking down here.

@guybedford
Copy link
Contributor Author

@balupton what are your thoughts on getting this merged? Just let me know if you still have any questions at all.

@balupton balupton mentioned this pull request Dec 9, 2015
@balupton
Copy link
Member

balupton commented Dec 9, 2015

@balupton what are your thoughts on getting this merged? Just let me know if you still have any questions at all.

Will merge and release sometime over the week.

balupton added a commit that referenced this pull request Dec 9, 2015
@balupton balupton merged commit 8361aaf into bevry:master Dec 9, 2015
@balupton
Copy link
Member

balupton commented Dec 9, 2015

Released to v1.1.5. Thanks for all your help on this.

I've also pushed up changes to esnextguardian and the modules that use it so far to implement your suggestions.

@guybedford
Copy link
Contributor Author

Great! Thanks for including this. The support for direct ES features and ES modules on npm will properly work in jspm 0.17 when that is released. Just let me know if you ever have any further questions in this area - it is an important and tricky interop to get right!

@balupton
Copy link
Member

@guybedford would you be able to post back here when it is safe for me to update our packages to have jspm point to the esnext versions instead of the es5 versions, cheers!

@balupton balupton changed the title Configure for jspm add jspm configuration Sep 8, 2016
@balupton
Copy link
Member

Reposting bevry/base#19 (comment)


Closing.

This is an issue for https://github.com/bevry/boundation

Regardless of that hoever, jspm (and all the other builders) should really add support for https://github.com/bevry/editions rather than everything having to add support for their specific thing (e.g. browser field, jspm field, module field, etc etc etc).

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