docs: add systemjs guide #464
Conversation
| "overrides": { | ||
| "npm:enzyme@2.3.0": { | ||
| "map": { | ||
| "cheerio": "@empty", |
There was a problem hiding this comment.
Not super sure about this line. Perhaps @guybedford knows the correct mapping to Webpack's 'window'?
|
As far as I know, SystemJS and jspm are distinct projects, with jspm being implemented on top of SystemJS. I don't think we should be including instructions for SystemJS, webpack, and jspm in one doc. |
|
SystemJS and JSPM are tightly coupled, but one is not built on the other (jspm is the package manager, systemjs is the loader). The reason to put these instructions in the same doc is that they are largely the same, and the reason for the workaround is also the same. What's the upshot to splitting them up? Updates to one are likely to require updating the other. |
|
SystemJS and jspm may be coupled, but why are we including it in the webpack documentation? If anything jspm specific instructions should be in a separate doc.
jspm is not a requirement for using webpack and I've honestly seen very few people using jspm + webpack + React. You may be able to pair the two, but our webpack docs shouldn't assume this. If we want to add docs for jspm we can mention webpack + jspm integration there. |
|
Note that I've renamed the file, so it's not "the webpack documentation". |
We have individual doc pages for other bundlers or build systems, we should keep that same setup. my point is that I don't want to lump together webpack and jspm. |
|
Oh, I didn't realize e.g. browserify was separate. Updated. |
tamird
changed the title
|
@aweary ping |
|
I don't know enough about SystemJS to know if this is accurate, unfortunately @ljharb @lelandrichardson does this look OK to you? |
|
We're using this configuration in CockroachDB: https://github.com/cockroachdb/cockroach/blob/master/ui/package.json#L51:L58 |
👍 I just want one other contributor to take a look at it before we make any changes to our docs. |
| } | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Rather than documenting this for all users to do manually, it may just be better to check this override in to the jspm registry or package.json file?
There was a problem hiding this comment.
the override depends on the react version in use.
|
Thanks @tamird for paving a path here. Are we really ok excluding these dependencies entirely in jspm, or should they be conditionally loaded based on certain conditions? If we can be sure of the configuration, then we can check it into the jspm registry (https://github.com/jspm/registry), or even better the package.json of this project itself, although either is fine. Just let me know if I can help further at all to review. |
|
they're conditional on the version of another dependency - is there a way to express that? |
|
Nevermind, after running |
|
@tamird yes we can do a conditional map configs, but it may be a little complex for this use case, so worth encouraging the manual process at least for now. |
|
@guybedford thanks for jumping in here, I'll defer to your judgement whether these docs look good for jspm/SystemJS. If there's anything enzyme specific you're not sure of I'm happy to help, but otherwise if this looks good I'm sure we can get this merged. |
| following files are labeled as "external", which means they will be ignored: | ||
|
|
||
| ``` | ||
| react/addons |
There was a problem hiding this comment.
since down below you specify the externals for react 0.14 and 15 specifically it's unclear for which version of react this list up here is meant for
There was a problem hiding this comment.
Yeah, this could be better. It's copied from the other guides, so I think that any improvement here should be bundled with improvements to the other guides.
|
@aweary I've taken a look and it looks alright to me (other than the 2 comments I left), however, i've no experience with systemjs so I can't be much help there |
|
@aweary I'm not that familiar with the exact mechanics for these configs, but it seems like the right sort of approach to me here although it would definitely be worth looking at automating this configuration in future. Thanks so much for being open to including instructions for this, and I'll aim to keep an eye on this use case as well. |
|
@tamird if you can address @nfcampos's comment(s) and if @guybedford thinks this looks acceptable I think we can merge this after you merge the latest changes from master. |
|
@aweary done. |
|
This LGTM. @tamird one last thing: You should add this file to : https://github.com/airbnb/enzyme/blob/master/docs/README.md so that it shows up in the docs site! |
|
Done. On Fri, Jul 8, 2016 at 12:22 AM, Leland Richardson <notifications@github.com
|
|
Thanks @tamird 🎉 |
Related to #302, jspm/jspm-cli#1875.