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

fix prerender and webpackconfig for custom directories in npm modules #117

Merged
merged 3 commits into from
Jun 20, 2017

Conversation

prateekbh
Copy link
Member

Fixes #42
This fixes inclusion of custom directories from node modules.
Also this means preact-material-components can now be used separately, without the entire package

@prateekbh prateekbh requested review from developit and lukeed June 16, 2017 06:52
@prateekbh prateekbh force-pushed the customDirectories branch 5 times, most recently from e476d00 to 03fc47f Compare June 16, 2017 07:14
@prateekbh
Copy link
Member Author

@developit @lukeed added a dummy travis yml to make travis happy

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

How does this work now that the include function has been removed? Are all node_modules processed by default? Unless I'm missing something obvious, doesn't seem like the module and/or jsnext:main entries will be picked up.

Also, can remove the @webpack-blocks/babel from package.json. 👍

Oh, and I'm pretty sure ignore:false operates the same as its default (ignore:null). So that can probably be removed.

@prateekbh
Copy link
Member Author

prateekbh commented Jun 16, 2017

How does this work now that the include function has been removed? Are all node_modules processed by default?

Yes, I see this kinda necessary cuz more and more modules with es6 code under module key will land soon(ESM support is pretty good now in browsers) and hence not processing them according to our presets/plugins might be a problem in user land

Unless I'm missing something obvious, doesn't seem like the module and/or jsnext:main entries will be picked up.

webpack picks node_modules in the order module -> js:next(i guess) -> main -> ...
so it will still be picked up

Also, can remove the @webpack-blocks/babel from package.json. 👍

sure will do

Oh, and I'm pretty sure ignore:false operates the same as its default (ignore:null). So that can probably be removed.

by default it is ignore: /node_modules/ so ignore: flase is needed

@prateekbh prateekbh force-pushed the customDirectories branch from 03fc47f to 30f9d15 Compare June 16, 2017 09:14
@prateekbh
Copy link
Member Author

prateekbh commented Jun 16, 2017

@lukeed i have made changes to package.json

@lukeed
Copy link
Member

lukeed commented Jun 16, 2017

Hmmm interesting. I'm seeing ignore:null here.

Also, the regex can be /\.jsx?$/.

Looks good~!

@prateekbh
Copy link
Member Author

@lukeed aren't you talking about ignores: null in prerender? Which is actually this
https://github.com/babel/babel/tree/master/packages/babel-register#ignores-node_modules-by-default

@lukeed
Copy link
Member

lukeed commented Jun 16, 2017

Oohhh... that's probably why I got stuck in my attempt! Thanks!

@prateekbh
Copy link
Member Author

Also @lukeed, i guess regex should be both. As node modules can be js as well. What say?

@lukeed
Copy link
Member

lukeed commented Jun 16, 2017

The ? applies to the character before in this case, so .js and .jsx files will both work. It's also the "standard" regex test that I see.

/\.jsx?$/.test('foo.js')
//=> true

/\.jsx?$/.test('foo.jsx')
//=> true

@prateekbh
Copy link
Member Author

@lukeed fixed the regex :)

@prateekbh
Copy link
Member Author

@developit need approval if all looks good.

@prateekbh
Copy link
Member Author

awaiting response/approvals here, lemme know if more work is needed here

@prateekbh prateekbh merged commit 2175efc into master Jun 20, 2017
@prateekbh prateekbh deleted the customDirectories branch June 20, 2017 11:44
@developit developit added this to the 1.2.0 milestone Jun 21, 2017
@developit
Copy link
Member

Hmm - I know I missed approving this, but can we double-check the removal of jsnext include()? In my experience this breaks ES5 modules since it rewrites them into strict mode - some modules assume the global context to be window, and when loaded via Babel it's undefined.

@prateekbh
Copy link
Member Author

@developit can you point me to some such modules?

@developit
Copy link
Member

Might be hard, only saw the issue in an internal project and it was ages ago.

For my own sake, how come the check for jsnext:main / modules was failing for preact-material-components?

@prateekbh
Copy link
Member Author

because for that repo, I suggest to include separate files instead of the whole repo as tree shaking is not yet perfect

@lukeed
Copy link
Member

lukeed commented Jun 21, 2017

It was because the preact-material-components's module entry points (and can only point) to a single index.js that looked (roughly) like this:

import Foo from './foo';
import Bar from './bar';
export { Foo, Bar }

This introduces the imperfect tree-shaking (nothing we can do, everything gets imported due to the declaration), so @prateekbh recommended nested-file imports.

However, the include() fn we had only respects the index.js file because preact-material-components is the module name, and not preact-material-components/foo.


On second thought, I think the include function could have stayed, but it wouldn't make a difference. The main reason this PR fixes #42 is because of ignore:false, which was ignoring all of node_modules by default.

It wouldn't have made a difference because ignore:false passes everything thru babel-loader (pros vs cons), which precludes the jsnext:main|module entries.

@prateekbh
Copy link
Member Author

@lukeed only ignore:false was still throwing error, i tried doing both separately, seems both are needed

@lukeed
Copy link
Member

lukeed commented Jun 21, 2017

Ah, okay. I guess include takes a higher priority. Thanks

@developit
Copy link
Member

It just seems odd the include() wouldn't have seen the module key within node_modules/preact-material-components/package.json and triggered Babel. Maybe I'm crazy :P

@prateekbh
Copy link
Member Author

@developit If i include components like

import Toolbar from 'preact-material-components/Toolbar

instead of

import {Toolbar} from 'preact-material-components

do you think it would have worked?

@lukeed
Copy link
Member

lukeed commented Jun 22, 2017

I would have expected it to, yes, but in my trials (using a different set of changes), it did not work for the reasons I mentioned above: only preact-material-components/index.js was transformed correctly.

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.

Build error with preact-material-components
4 participants