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

babel script: 'import' in tag script will get error #21

Closed
txchen opened this issue Nov 11, 2015 · 30 comments
Closed

babel script: 'import' in tag script will get error #21

txchen opened this issue Nov 11, 2015 · 30 comments

Comments

@txchen
Copy link

txchen commented Nov 11, 2015

environment: riot 2.3.1, webpack latest, babel v6.
tag example:

<mytag>
  <script type="babel">
  import foo from './bar'
  // some other fancy code
  </script>
</mytag>

This will get error when running babel: error 'import' and 'export' may only appear at the top level

@appedemic
Copy link
Contributor

This is caused by the babel parser. babel 6 doesn't allow toplevel this and destroys references (#15) so the parser wraps code in a function. The import statements are executed within this wrapper function and that is causing the error.

I can't think of an easy solution right now. The two hard solutions I can think of right now are:

  • Remove the wrapper function. This implies that preset es2015 cannot be used because the es2015-plugin-modules-commonjs was causing babel compiler destroys this references within a tag #15. This also implies that end-users cannot simply do "npm install babel-preset-es2015" but have to install all the plugins from the preset manually (except commonjs). Or maybe we can do some trick witha .babelrc?
  • Add some dirty code to extract/hoist import statements from the code block and move them out of the wrapper function.

@txchen
Copy link
Author

txchen commented Nov 11, 2015

I think previously it works because of the blacklist 'strict' setting, not sure if babel 6 still have something similar.

@aMarCruz
Copy link
Contributor

@appedemic , it is true. I'm not a babel user, but in tests babel-core 5.8.x was the cleaner and faster solution, v6 was complicated and slower.

@txchen
Copy link
Author

txchen commented Nov 12, 2015

@aMarCruz right, I also noticed that v6 is slower, but seems v5 is not maintained any more, I think we still need to go with v6.

@aMarCruz
Copy link
Contributor

It seems so. I'm closing this issue by now.

@txchen
Copy link
Author

txchen commented Nov 12, 2015

@aMarCruz why close this? We don't want to support babel v6, then?

@GianlucaGuarini
Copy link
Member

@aMarCruz I want to reopen this issue I think we can fix this as well.
@txchen have you tried also using the include outside of your tags?

import foo from './bar'
<mytag>
  <script type="babel">
  // some other fancy code
  </script>
</mytag>

I will try asking support on the babel repo

@aMarCruz aMarCruz reopened this Nov 12, 2015
@GianlucaGuarini
Copy link
Member

I have just seen that babeljs has already fixed the issue we just need to implement this option 🎉

@aMarCruz
Copy link
Contributor

👍

@appedemic
Copy link
Contributor

Yeah, "Add allowTopLevelThis option to babel-plugin-transform-es2015-modules-commonjs" 💃

I can probably create a patch today

@appedemic
Copy link
Contributor

Currently there is no way to add plugin options to a preset, so this allowTopLevelThis cannot be used directly with presets:['es2015']. There is an ongoing discussion about how to support this...

@aMarCruz
Copy link
Contributor

@appedemic , thanks. we hope your PR.

@txchen
Copy link
Author

txchen commented Nov 12, 2015

@GianlucaGuarini putting import in the very beginning of the tag file does not work, riot-loader will throw error:

ERROR in ./src/view/detail-view.html
Module parse failed: /Users/txchen/GithubCode/feplay/riot_webpack/node_modules/riotjs-loader/index.js!/Users/txchen/GithubCode/feplay/riot_webpack/src/view/detail-view.html Line 1: Unexpected token
You may need an appropriate loader to handle this file type.
| import stores from '../stores'
| riot.tag2('detail-view', '<h2>{_post.title}</h2> <p>{_post.content}</p> <p>{_post.likes} Likes</p> <a if="{opts.data > 1}" href="#detail/{opts.data - 1}">Previous Post</a> | <a if="{opts.data < _total}" href="#detail/{opts.data - -1}">Next Post</a>', '', '', function(opts) {
|   var _this = this;
 @ ./src/riotTags.js 9:0-34

@GianlucaGuarini
Copy link
Member

I've asked support to the babel maintainers babel/babel#3055 I hope to get a valid answer soon

@appedemic
Copy link
Contributor

For fun I created a modified es2015 preset: https://github.com/appedemic/babel-preset-es2015-riot

However, npm has some trouble accepting logins right now so I can't publish this as a package right now

@andyrj
Copy link

andyrj commented Nov 23, 2015

is babel-preset-es2015-riot still going to be pushed to npm?

Just tested and as far as I can tell even using this allowTopLevelThis option doesn't seem to fix this error. :(

So for now stay with babel v5.x I suppose?

@appedemic
Copy link
Contributor

Right now it's not possible to send the allowTopLevelThis option to the right plugin within the preset. I've been thinking, if we add an option to the compiler where the end user can supply a list of presets/plugins to use, it would be the most flexible.

For example, I really like to use some ES7 features (async/await and Object.entries) and they are not part of the es2015 preset, but stage-0 and stage-3. For these features I need to add a preset/plugin to the babel compiler.

I could create a pr that takes the compiler plugins/preset options from the configuration and defaults to es2015. Then, I can supply my desired configuration into riotify and it will be passed on nicely.

@aMarCruz
Copy link
Contributor

There is an idea of a possible system of plugins that should solve some issues like this in riot/riot#1272.
It is likely that something like that is implemented.

@appedemic
Copy link
Contributor

Ok, so the plan is to make packages for the parsers? So to have some npm packages like riot-compiler-parser-babel and riot-compiler-parser-jade, etc.? And provide some mechanism to configure the compiler with said packages?

riot.compiler.use(require('riot-compiler-parser-babel')({ presets: ['es2015'] }))

If implemented correctly it would be a nice future-ready solution

@aMarCruz
Copy link
Contributor

@appedemic , yeah, I hope soon, after stabilizing the current version, the changes have been many.

@GianlucaGuarini
Copy link
Member

Ok this issue was solved in the es6 branch @aMarCruz can you review my changes and make the new releases of the riot-tmpl and riot-compiler? I would like compile riotjs using rollup and also update the cli as well adding the debug compiler option

@aMarCruz
Copy link
Contributor

aMarCruz commented Dec 6, 2015

@GianlucaGuarini sure. the dist/riot.*.js files will be generated anyway?

@GianlucaGuarini
Copy link
Member

Let's keep it for now we will delete it if everything will run smooth

@aMarCruz
Copy link
Contributor

aMarCruz commented Dec 6, 2015

ok

@GianlucaGuarini
Copy link
Member

@aMarCruz can we infer the url param for the compile method from the first one ( src ) at least on node?

@GianlucaGuarini
Copy link
Member

@appedemic can we use your riot-bable preset in our official repo? I woul like to publish it on npm with the new release of the compiler

@aMarCruz
Copy link
Contributor

aMarCruz commented Dec 7, 2015

@GianlucaGuarini , no way to infer the url from the src parameter, it can come from anywhere.

@GianlucaGuarini
Copy link
Member

@appedemic I have published on npm the riot-preset based on your setup thank you and feel free to pull request on this repo

@creatorrr
Copy link

@appedemic I thought the function wrapper was only necessary for this bug. The top-level this issue should be resolved now with the babel-riot preset. Removing the wrapper function from the compiled templates should allow imports.

If not, for now we could revert to using require( _module ) in tags instead of import.

@GianlucaGuarini
Copy link
Member

@creatorrr you are right this bug was fixed and it can be closed

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

No branches or pull requests

6 participants