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

Options style support customize & update README.md #221

Merged
merged 3 commits into from
Apr 4, 2018

Conversation

GeoffZhu
Copy link
Contributor

fix issue #220

@yesmeck
Copy link
Contributor

yesmeck commented Mar 30, 2018

Could you add a testcase?

src/Plugin.js Outdated
@@ -58,8 +58,8 @@ export default class Plugin {
this.selectedMethods[methodName] = addDefault(file.path, path, { nameHint: methodName });
if (style === true) {
addSideEffect(file.path, `${path}/style`);
} else if (style === 'css') {
addSideEffect(file.path, `${path}/style/css`);
} else if (typeof style === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewrite typeof as a function typeof() ? || Define function isString?

Copy link
Contributor

@afc163 afc163 Apr 4, 2018

Choose a reason for hiding this comment

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

I mean

style: (name) => `${name}/style`
style: (name) => `${name}/style/css`

like #163

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@GeoffZhu
Copy link
Contributor Author

Testcase added

@yesmeck yesmeck merged commit 2929996 into umijs:master Apr 4, 2018
@GeoffZhu
Copy link
Contributor Author

GeoffZhu commented Apr 8, 2018

@yesmeck @afc163
I try 1.7.0 in my project. There was a serious mistake. I'm so sorry about than.

Reason:
In Babel docs of .babelrc

All Babel API options except the callbacks are allowed (because .babelrc files are serialized as JSON5).

So, make option style as a Function lead to some bugs. Babel will throw Error 'Not parse JSON'.

Can I create a new pr fix this bug?

@yesmeck
Copy link
Contributor

yesmeck commented Apr 8, 2018

It's obvious, you can not write a function in JSON, how do you plan to fix it?

@GeoffZhu
Copy link
Contributor Author

GeoffZhu commented Apr 8, 2018

@yesmeck Back to my previous commit 3590aca

if (style === true) {
  addSideEffect(file.path, `${path}/style`);
} else if (typeof style === 'string') {
  addSideEffect(file.path, `${path}/style/${style}`);
}

If you don't accept it, I will find another way.

@yesmeck
Copy link
Contributor

yesmeck commented Apr 8, 2018

Though you can not write function in JSON, you can still use function in the config of webpack for babel-loader, so I think it's ok to keep it as it is.

@GeoffZhu
Copy link
Contributor Author

GeoffZhu commented Apr 8, 2018

OK, thx

zhanguangao pushed a commit to zhanguangao/babel-plugin-module-federation-import that referenced this pull request Sep 1, 2023
Options style support customize & update README.md
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.

3 participants