Build error with preact-material-components

[cedric-marcone]

Hello,

Linking a component from preact-material-components works like a charm in dev mode but when trying to build for production it fails.

Steps to reproduce

1. preact create test
2. cd test
3. npm install --save preact-material-components
4. edit routes/home/index.js
5. paste this code :

import { h, Component } from 'preact'
import style from './style'

import FormField from 'preact-material-components/FormField'
import Radio from 'preact-material-components/Radio'

import 'preact-material-components/FormField/style.css'
import 'preact-material-components/Radio/style.css'


export default class Home extends Component {
  render() {
    return (
      <div class={style.home}>
        <h1>Home</h1>
        <FormField>
          <Radio id="r1" name='opts'></Radio>
          <label for="r1">Radio 1</label>
          <Radio id="r2" name='opts'></Radio>
          <label for="r2">Radio 2</label>
        </FormField>
      </div>
    )
  }
}

=> npm start : OK

=> npm run build : Fails

Template execution failed: SyntaxError: Unexpected token import
  Error: /Users/cedric/Desktop/test/node_modules/preact-material-components/FormField/index.js:1
  (function (exports, require, module, __filename, __dirname) { import { h } from "preact";
                                                                ^^^^^^
  SyntaxError: Unexpected token import
  
  - node.js:152 Object.require.extensions.(anonymous function) [as .js]
    [test]/[babel-register]/lib/node.js:152:7
  
  - module.js:20 require
    internal/module.js:20:19
  
  - index.js:4 Object.<anonymous>
    /Users/cedric/Desktop/test/routes/home/index.js:4:1
  
  - node.js:144 loader
    [test]/[babel-register]/lib/node.js:144:5
  
  - node.js:154 Object.require.extensions.(anonymous function) [as .js]
    [test]/[babel-register]/lib/node.js:154:7
  
  - module.js:20 require
    internal/module.js:20:19
  
  - index.js:7 Object.<anonymous>
    /Users/cedric/Desktop/test/index.js:7:1
  
  - node.js:144 loader
    [test]/[babel-register]/lib/node.js:144:5

Any idea ?

[cedric-marcone]

Oops, BTW,
preact --version
1.0.1

[lukeed]

Thanks!

@developit I'm pretty sure it has to do with the node_modules include-r. The package has a module entry pointing to a index.js, but importing single components (eg, FormField) traverses deeper within the node_module directory, which no longer matches the module entry.

Although this method "expects" index.js to have something like:

export { Tabs, Menu, FormField, ... }

... and to be imported via

import { FormField } from 'preact-material-components'

... I don't think @prateekbh is wrong to export components the way he has. So, perhaps (??), we should include the entire module directory if we can resolve a module or jsnext:main entry, rather than the entries themselves?

[lukeed]

@cedric-marcone It looks like preact-material-components does export components the way I've described above.

So, I know the documentation says to do what you've done, but I'd try this & see if it works:

import  { FormField, Radio } from 'preact-material-components'
import 'preact-material-components/FormField/style.css'
import 'preact-material-components/Radio/style.css'

PS: I'm just guessing, but it looks like it should be fine.

[cedric-marcone]

Thanks !

Your suggestion didn't work right away because there is an inconsistency in preact-material-components regarding the naming of the FormField component (FormField vs Formfield) => I'll report there ;)

I tried with the following and it works perfectly :

import {Formfield, Radio} from 'preact-material-components'
import 'preact-material-components/FormField/style.css'
import 'preact-material-components/Radio/style.css'

The only problem I can see is that initial method of importing components was advertized as the best way to prevent bundling unused components.

Am I wrong ?

[lukeed]

Cool! I'm pretty certain that because it's using the module syntax, webpack will treeshake the rest away.

The best way to check, though, is to do a before-after and check the bundle size of each. That's what I would do.

[cedric-marcone]

I'll do it.
Thanks for your help !

[prateekbh]

@lukeed @cedric-marcone So here's a few pointers

1. Yes including a component separately is the best way to do it. @lukeed sadly webpack does not tree-shake classes :(
2. @cedric-marcone so sorry for the errornous exports, will fix them right away.
3. Most probably the reason for this is we're excluding node_modules, which is sad because as we expect more ESM to land in node repo and are not willing to transpile it.(this assumption of mine can be false, but i'll cross check and let you know if it is true.)

[prateekbh]

Also i could really use as a ver good data point if you want the individual components to be transpiled into es5 format as well(which will make me sad though).

[rkostrzewski]

1. Yes including a component separately is the best way to do it. @lukeed sadly webpack does not tree-shake classes :(
@prateekbh @lukeed FYI webpack treeshakes classes but can't handle following.
Imagine module named 'lib':

import { A } from './a.js'
import { B } from './b.js'

export { A, B }

using it as so import { A } from 'lib' won't remove B because of re-export which UglifyJS can't handle. Which is basically every npm module written in ES6+. 😢

babel/babili aims to tackle that problem - might be worth including inside webpack config for smaller bundle sizes. (personally haven't used it so I don't know what challenges come with it)

[prateekbh]

@rkostrzewski thanks for pointing it out! It makes total sense to me now.
I am for now following this bug on webpack
webpack/webpack#2899

but even with above info the need to add components separately does stand valid.

[lukeed]

Ah, right! That's why I felt hesitant.

I do believe that this approach solves the issue. I've used it in a few projects and I think I remember it being beneficial:

export { default as Foo } from './foo'
export { default as Bar } from './bar'

[prateekbh]

@lukeed lemme try this, if this works it will make using preact-material-components even easier.

[lukeed]

Sure! Please let me know. It's been a while since I stumbled on it, not sure if my memory serves correctly. =P

[prateekbh]

😞 sry but the above doesn't help, everything is still in the final bundle

[prateekbh]

we should include the entire module directory if we can resolve a module or jsnext:main entry, rather than the entries themselves?
@lukeed @developit I tried looking into the node-modules include-r but couldn't wrap my head around it much.

For the components part, I guess at this stage it does demand to be included separately. Please let me know if I can be any help here, I'll try if I can fix and send a PR but could really use some help

[developit]

hmm - babel should be enabled within node_modules for anything with a module entry, which it looks like preact-material-components has... I wonder why it's not working?

[prateekbh]

just a note, this function is returning true from first condition itself, but not transpiling them

[thangngoc89]

I can confirm this with linkState ultility. Just import it to anyfile and you'll see syntax errors

[developit]

Hmmm. I wonder if the babel config is manually excluding node_modules.

[prateekbh]

ok so I confirmed this by playing with preact-router.

1. node_modules>preact-router>package.json> delete"main key"> run build... ERROR
2. node_modules>preact-router>package.json> copy"module key" to "main key"> run build... ERROR

[developit]

so in either case it's not getting babel-loader applied?

[prateekbh]

yep, in both cases, any es6 found in node_modules throws error.

[developit]

k. something's up with that plugin.

this might be related:
https://github.com/babel/babel/tree/master/packages/babel-register#ignores-node_modules-by-default

[prateekbh]

@developit are we using this in webpack?
I only see this running in some prerender file.

[prateekbh]

@developit we are trying to run npm run build is this plugin executed from that command anywhere?
I can t seems to find any such execution path

[developit]

prerendering is kicked off from a function call within the HTML template. It's handled in this file:
https://github.com/developit/preact-cli/blob/master/src/lib/prerender.js

[prateekbh]

@developit i did tried changing that however i got following error

Template execution failed: Error: Options {"loose":true,"modules":"commonjs","uglify":true,"browsers":["> 1%","Last 2 versions","IE >= 9"],"exclude":["transform-regenerator","transform-es2015-typeof-symbol"]} passed to /Users/prateekbh/projects/preact-cli/node_modules/babel-preset-env/lib/index.js which does not accept options. (While processing preset: "/Users/prateekbh/projects/preact-cli/node_modules/babel-preset-env/lib/index.js") (While processing preset: "/Users/prateekbh/projects/preact-cli/node_modules/babel-preset-env/lib/index.js")
  Error: Options {"loose":true,"modules":"commonjs","uglify":true,"browsers":["> 1%","Last 2 versions","IE >= 9"],"exclude":["transform-regenerator","transform-es2015-typeof-symbol"]} passed to /  Users/prateekbh/projects/preact-cli/node_modules/babel-preset-env/lib/index.js which does not accept options. (While processing preset: "/Users/prateekbh/projects/preact-cli/node_modules/babel-  preset-env/lib/index.js") (While processing preset: "/Users/prateekbh/projects/preact-cli/node_modules/babel-preset-env/lib/index.js")

[prateekbh]

so it seems,,
removing loose option here and adding ignore: false will make it work..
not sure if we can remove option loose.

if yes i'll raise a PR

[developit]

99% sure this is fixed by #63 - @lukeed found that without exclude:[], babel-loader will always ignore node_modules.

[lukeed]

Yup. However, I think that in this case (unless @prateekbh has updated his module?), we are only importing & paying attention to the index.js since it's the entry.

Later tonight, I want to check that our custom include() will allow us to import components the way OP described:

import FormField from 'preact-material-components/FormField'
import Radio from 'preact-material-components/Radio'

[prateekbh]

@lukeed i havent really changed anything in the module, because tree shaking isn't perfect so far. I will check if the cuatom import works

[prateekbh]

@lukeed it is failing with this error:

route-home.chunk.52b99.js from UglifyJs
Unexpected token: name (Select) [route-home.chunk.52b99.js:77,6]

however when i tried removing uglify from all the places it fell back to the same error as earlier.

[lukeed]

I've been playing with it for longer than I'd like to admit. At this moment, I'm convinced that the @webpack-blocks/babel6 package is enforcing/constraining too much. This is also the reason why that exclude issue popped up (doesn't happen if using babel-loader directly).

[prateekbh]

@lukeed also I must admit, after banging my head with webpack for a while I just started feeling comfortable with loaders and then came webpack-blocks... 😭 😭 😭 😭

[prateekbh]

@lukeed so are we changing anything then?

[AbrahamAlcaina]

I have the same issue @prateekbh .
(my comment to watch the conversation)

[lukeed]

@prateekbh @developit I would like to move from the @webpack-blocks/babel6 wrapper to babel-loader directly.

However, I was still struggling to include custom directories. I was also looking into this built-in but couldn't quite figure it out either. The idea with the latter is to move contexts when webpack recognizes an import is coming from within a node_module.

@developit Should maybe cut the new release now & get this fix in as soon as it's solved. Altho it is important, it's not big enough to delay all the other improvements that have been made.

[prateekbh]

@prateekbh @developit I would like to move from the @webpack-blocks/babel6 wrapper to babel-loader directly.

This sounds good to me, also with loader if you do no say excludes: /node_modules/ it includes all the node_modules and their internal directory.

@developit Should maybe cut the new release now & get this fix in as soon as it's solved. Altho it is important, it's not big enough to delay all the other improvements that have been made.

definitely we should not wait for this, we'll do a minor version bump later on.

[prateekbh]

@lukeed is it ok if I PR to make it to babel-loader?
also can you help me a little where shall i put it?

[lukeed]

@prateekbh Yeah, I think so. I believe @developit signed off on it too.

As for doing it, you can modify this group to be a customConfig.