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

Updating to [email protected] #45

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

wongwill86
Copy link

I had to update the package on my fork because [email protected] reloading did not trigger parent dependency updates for plugin-text (i.e. module that imports text file does not update when text file is updated). Updating to v1.1.0 seems to fix this issue.

Here's a PR in case you were planning to update these dependencies.

Updating to systemjs-hot-reloader@ 1.1.0 requires updating jspm/systemjs.

New breaking changes in SystemJS@ 0.20.x due to NodeJS ES Module directions http://guybedford.com/systemjs-alignment.

  • That is, import { name } from 'cjs.js', where cjs.js is a CommonJS module will no longer be supported, and instead will require import cjs from 'cjs.js'; cjs.name. `

  • Now cjs modules need to be compiled with __esModules in order for import { name } from 'cjs.js'

  • Unit testing seemed to have broke with

    ReferenceError: System is not defined
        at Object.<anonymous> (src/store/currency-rates/spec/currency-rates-reducer.spec.tsx:1:119)
    
    • For some reason adding moduleNameMapper seemed to allow Jest to resolve this

Also, side note, Jest doesn't seem to support JSPM/Systemjs correctly :
http://stackoverflow.com/questions/32945193/jest-testing-with-es6-jspm-systemjs-reactjs

@piotrwitek
Copy link
Owner

piotrwitek commented Mar 6, 2017

Hello,
Thanks for PR, there is existing issue for this: #39
I will try to test it if working.

What I don't like with this PR is the change of React import.
The syntax you are introducing is not compliant with the standard ES Modules.
It is because from consumer perspective I want to import React as namespace, and the correct syntax to import namespace is import * as React.

@@ -107,7 +107,6 @@ SystemJS.config({
"url": "npm:[email protected]",
"util": "npm:[email protected]",
"vm": "npm:[email protected]",
"whatwg-fetch": "npm:[email protected]",
Copy link
Owner

Choose a reason for hiding this comment

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

no you cannot remove it, types was added to types lib, not the polyfill

Copy link
Author

Choose a reason for hiding this comment

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

sorry! added it back in!

Copy link
Owner

Choose a reason for hiding this comment

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

no problem, cool :)

scripts/build.js Outdated
@@ -2,7 +2,8 @@ require('shelljs/global');

const package = require('../package.json');
const prodDependencies = Object.keys(package.jspm.dependencies);
const devDependencies = Object.keys(package.jspm.devDependencies);
const devDependencies = Object.keys(package.jspm.devDependencies)
.filter((package) => package !== "systemjs-hot-reloader");
Copy link
Owner

Choose a reason for hiding this comment

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

this is unnecessary, as you need this in dev hot reloading

Copy link
Author

Choose a reason for hiding this comment

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

this was a workaround for an issue i found with bundling systemjs-hot-reloader.
alexisvincent/systemjs-hot-reloader#138

I'll see if i can get a reply on what needs to be configured.

Copy link
Owner

Choose a reason for hiding this comment

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

ah I see, good to know, I gonna try as well if it works for me

@@ -1,6 +1,6 @@
// lib imports
import * as React from 'react';
import * as ReactDOM from 'react-dom';
import React from 'react';
Copy link
Owner

Choose a reason for hiding this comment

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

import statements should be changed back to namespace, as we want to import namespace and not default export

Copy link
Author

Choose a reason for hiding this comment

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

Hrmm. I had a lot of problems with this. I updated systemjs/builder#775 to see if i can get some help to get the namespace import working again.

Copy link
Owner

Choose a reason for hiding this comment

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

@wongwill86 I think that if React is not supporting ES Module flag we should probably stick to default import, this is really annoying because we will not be able to use named import as well, but unfortunately I couldn't find any good workaround ;/

import { connect } from 'react-redux';
import { returntypeof } from 'react-redux-typescript';
import RRT from 'react-redux-typescript';
Copy link
Owner

Choose a reason for hiding this comment

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

named import is better

@piotrwitek
Copy link
Owner

there is a bigger issue going on regarding commonjs modules: systemjs/systemjs#1587

@piotrwitek
Copy link
Owner

@wongwill86 thanks for contribution
As already stated here: #39, I gonna wait with 0.20 upgrade until this issue is resolved: systemjs/systemjs#1587

For me it seems 0.20 update introduce too much issues for existing users of boilerplate that it is not worth currently.

Thanks for help

@wongwill86
Copy link
Author

no problem i hope it helps. That makes sense. thanks for reviewing!

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