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

Not support for Rollup in AOT mode #85

Closed
miao17game opened this issue Aug 18, 2017 · 14 comments · Fixed by #240
Closed

Not support for Rollup in AOT mode #85

miao17game opened this issue Aug 18, 2017 · 14 comments · Fixed by #240
Assignees
Labels
🙅 Won't Fix We are not going to fix or resolve it

Comments

@miao17game
Copy link

I'm submitting a...


[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Support request => Please do not submit support request here

Current behavior

When i try to rollup for the AOT mode, i find that it's impossible to build with ng-zorro.

the error message is below here:


> ngc -p tsconfig-aot.json && rollup -c rollup-config.js

⚠️   'default' is imported from external module 'rollup' but never used

'default' is not exported by 'node_modules\@angular\common\@angular\common.es5.js'
'default' is not exported by 'node_modules\@angular\core\@angular\core.es5.js'
�   Error: Cannot call a namespace ('moment')
node_modules\ng-zorro-antd\src\release\datepicker\nz-datepicker.component.js (17:26)
15:     this._value = null;
16:     this._today = new Date();
17:     this._selectedMonth = moment(this.nzValue).month();
                              ^
18:     this._selectedYear = moment(this.nzValue).year();
19:     this._selectedDate = moment(this.n

Expected behavior

Minimal reproduction of the problem with instructions

What is the motivation / use case for changing the behavior?

the problem is the 'moment', it's imported in time-picker component in this format:


import * as moment from 'moment'

but it's not support in AOT mode.
if i change the format into


import moment from 'moment'

,no errors will be called up, but still some other errors in moment.
if i remove the NG-ZORRO from my angular4 project, there is nothing wrong.
And there is no problems in JIT mode.

I don't think change the source by myself is a good way to resolve this problem.
I need help.

Environment


Angular version: 4.2.4

ng-zorro-antd version: 0.5.0

Browser:
- [x] Chrome (desktop) version 59
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] IE version XX
 
For Tooling issues:
- Node version: XX  
- Platform: Windows7 

Others:

@miao17game
Copy link
Author

well, i have gotten a new way to resolve this problem, the is the plugins section in my rollup-config:


plugins: [
        nodeResolve({ jsnext: true, module: true }),
        commonjs({
            namedExports: {
                'node_modules/cerialize/index.js': ['deserialize', 'serializeAs', 'deserializeAs', 'serialize', 'inheritSerialization'],
                'node_modules/angular2-cookie/core.js': ['CookieOptions', 'CookieService'],
            },
            include: [
                'node_modules/rxjs/**',
                'node_modules/angular2-cookie/**',
                'node_modules/cerialize/**',
                'node_modules/ng-zorro-antd/**',
                'node_modules/moment/**',
            ]
        }),
        {
            name: 'replace moment imports',
            transform: code =>
                ({
                    code: code.replace(/import\s*\*\s*as\s*moment/g, 'import moment'),
                    map: { mappings: '' }
                })
        },
        uglify()
    ]

if not add in commonjs, there will be a new undefined-error throwed in runtime.

@vthinkxie
Copy link
Member

vthinkxie commented Aug 18, 2017

angular cli is officially supported, rollup has not been tested yet.
thanks for feedback, we will discuss it later.

@trotyl
Copy link
Contributor

trotyl commented Aug 18, 2017

Sounds valid, the current usage is not valid JavaScript code and has been banned in TypeScript (microsoft/TypeScript#2242 (comment)) for a long time, I'm not sure why it could still compile.

@LinboLen
Copy link
Contributor

LinboLen commented Aug 18, 2017

  • don't import default in angular, the tslint have hinted it with error.

import moment from 'moment this is import default which corresponding to export default xxx

image

  • import moment sames that it's a hack style

Updated

correctly default import support by rollup, but not import * as moment from moment.
which tsconfig.json config compilerOptions allowSyntheticDefaultImports: true will resolve issue

Use

import moment from 'moment';
all tsconfig*.json compilerOptions add allowSyntheticDefaultImports: true

Updated Again

no better way to use moment. There is a consensus that use import as moment from 'moment' suit for users, tsc, ngc. and replace import as moment from 'moment' as import moment from 'moment' when using rollup

@trotyl
Copy link
Contributor

trotyl commented Aug 18, 2017

@LinboLen There is no definition of how ES module interacts with CommonJS, for native Node.js and earlier version of Babel (by default), the module.exports would be retrieved from import foo from '...', but for TypeScript (by default) and latest version of Babel (by default), the module.exports would be retrieved from import * as foo from '...'.

Moreover, TypeScript has provided allowSyntheticDefaultImports option for type checking to allow default import for namespace implicitly, but must be combined with other bundle tools (like webpack).

Frankly, there is no valid way to import something like moment or express, all of them would be wrong, just some may work and some may not.

@LinboLen
Copy link
Contributor

LinboLen commented Aug 18, 2017

long time i have not tracking ecmascript import feature.

onething i can confirm. should only use import {foo} from "bar", other like import foo from "bar" import * as foo from "bar" is not recommend

i know node style, but right now should only use import {foo} from "bar"

i know moment, really heart me ever.(i use import * as moment from "moment", later i dropped it)

ref
moment conflict

@miao17game
Copy link
Author

the problem is there are functions and namespace in this library with the same declare name 'moment', and the rollup can not the difference.


�   Error: Cannot call a namespace ('moment')

in fact, the 'moment' to be used is a function ,not a namespace... but it seems that rollup don't know it

@trotyl
Copy link
Contributor

trotyl commented Aug 18, 2017

Whatever import by as must be a namespace, that's the semantics of JavaScript language, and CommonJS module.exports has no equivalence in ES module. Rollup did nothing wrong for following the standard as an ES module bundler.

@miao17game
Copy link
Author

but we need a solution here, or the experience in AOT and rollup won't be improved.

@miao17game
Copy link
Author

my opinion is whether a hack or reconstruction in source code of ng-zorro isfeasible, but not the most important.
the first work to do is make the ng-zorro usable in AOT/rollup mode cause this mode is better than any other ways for the production.

thst's my hack way ,though it' not elegant, and i prefer an official solution if possible.

@trotyl
Copy link
Contributor

trotyl commented Aug 18, 2017

The best way to do is to make moment a global requirement like a polyfill instead of a module dependency, as it's not tree-shakable at all.

@wilsoncook wilsoncook self-assigned this Aug 19, 2017
@wilsoncook
Copy link
Member

wilsoncook commented Aug 21, 2017

Even i tried pass through the rollup compiling with success, the generated build.js file executed with errors that can't resolve. We recommend using angular-cli's tree-shaking functionality because these functions update too quickly.

@trotyl
Copy link
Contributor

trotyl commented Sep 7, 2017

Will be solved by #240.

@lock
Copy link

lock bot commented Feb 19, 2019

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🙅 Won't Fix We are not going to fix or resolve it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants