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

Add @babel/runtime #692

Closed
wants to merge 1 commit into from
Closed

Add @babel/runtime #692

wants to merge 1 commit into from

Conversation

juliusknorr
Copy link
Contributor

@juliusknorr juliusknorr commented Oct 26, 2019

Fixes #691

According to https://babeljs.io/docs/en/babel-plugin-transform-runtime @babel/plugin-transform-runtime requires @babel/runtime as a production dependency.

@skjnldsv @ChristophWurst I though babel/polyfill should no longer be needed. Still seems to work fine with IE11 if the app is taking care of a proper babel tranformation. It was not imported in @nextcloud/vue anyway.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr added bug Something isn't working 3. to review Waiting for reviews labels Oct 26, 2019
@juliusknorr juliusknorr added this to the 1.1.1 milestone Oct 26, 2019
@skjnldsv
Copy link
Contributor

I'm not sure we still needs the babel/plugin-transform-xxx to be honest.
We should probably just import core-js and use preset-env with a proper useBuiltIns config 🤔

@korelstar
Copy link
Contributor

Why does nextcloud-vue need babel/core-js at all? Isn't it sufficient if the apps using nextcloud-vue use babel/core-js?

@skjnldsv
Copy link
Contributor

skjnldsv commented Oct 26, 2019

Why does nextcloud-vue need babel/core-js at all? Isn't it sufficient if the apps using nextcloud-vue use babel/core-js?

They need to specifically specify to babel to include the vue components then :)
It's much easier to make sure we're compatible with the same config here than to put all the transpile weight on the apps.
https://github.com/nextcloud/server/blob/c280eff9b5daa0cf85322b00e38662c1ff4fc7bd/webpack.common.js#L83

@raimund-schluessler
Copy link
Contributor

I don't really know how to test this. Checking out the branch, running make and copying the dist folder to tasks\node_modules\@nextcloud\vue\dist\ does not solve the issue. Also, replacing "@babel/polyfill": "^7.4.4", with "@babel/runtime": "^7.6.3", in tasks\node_modules\@nextcloud\vue\package.json does not help.

@juliusknorr
Copy link
Contributor Author

Ok I guess in that case we should make sure that tthe required transpiling packages are not excluded in the wepack config since #679 just excluded all

korelstar
korelstar previously approved these changes Oct 27, 2019
Copy link
Contributor

@korelstar korelstar left a comment

Choose a reason for hiding this comment

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

#691 did also appear for me. After using this PR the issue was fixed. I'm not sure if the steps by @raimund-schluessler are sufficient to test this. Here is what I did:

  • in my local nextcloud-vue clone, checkout this branch
  • execute npm install, make build-js-production, npm link
  • in my app's directory, execute npm link @nextcloud/vue, make build-js
  • everything is fine :-)

@korelstar korelstar dismissed their stale review October 27, 2019 09:20

my tests where not sufficient, now the error is not reproducable any more

@korelstar
Copy link
Contributor

Okay, I did the test wrong. But I think adding @babel/runtime to the app's dependencies solves the issue. In order to enforce apps to install @babel/runtime, we could add it as peer-dependeny to nextcloud-vue. Wouldn't that solve the issue?

@raimund-schluessler
Copy link
Contributor

#691 did also appear for me. After using this PR the issue was fixed. I'm not sure if the steps by @raimund-schluessler are sufficient to test this. Here is what I did:

* in my local `nextcloud-vue` clone, checkout this branch

* execute `npm install`, `make build-js-production`, `npm link`

* in my app's directory, execute `npm link @nextcloud/vue`, `make build-js`

* everything is fine :-)

Following this seems to fix the issue for Tasks. 👍

@skjnldsv
Copy link
Contributor

if the app is taking care of a proper babel tranformation.

this does not happen.
Remember that the app should not be in charge of transpiling the node_modules. This should be done only on very rare specific occasions :)

@juliusknorr
Copy link
Contributor Author

Remember that the app should not be in charge of transpiling the node_modules. This should be done only on very rare specific occasions :)

I actually think this should be the apps responsibility. Other libraries also just ship their ES6 modules and the app needs to handle making them compatible to the browser stack that should be supported. General transpiling is done in @nextcloud/vue (e.g. for icon fonts) but we should do babel transformations ourselves.

@skjnldsv
Copy link
Contributor

so, what is the status here? :)

@korelstar
Copy link
Contributor

I agree with @juliushaertl and vote for removing babel/corejs from @nextcloud/vue and let the apps do this part. Typically, they have to do it anyway (for their own code).

@skjnldsv
Copy link
Contributor

As a comparison, out datepicker recently release 3.0.0, where they added babel to be compatible with ie. I think it's less trivial to have default working deps than having to do webpack trick to include specific packages in the transpiler.

https://github.com/nextcloud/server/blob/4ddea4bdfbc36e13f7b397a1d881ddff2d364419/webpack.common.js#L86

@ChristophWurst
Copy link
Contributor

As a comparison, out datepicker recently release 3.0.0, where they added babel to be compatible with ie. I think it's less trivial to have default working deps than having to do webpack trick to include specific packages in the transpiler.

Second that. I would also argue that we should transpile this lib already, given that we have a shared browserslist config and thus know what apps most likely want to be compatible with.

@ChristophWurst
Copy link
Contributor

We should probably just import core-js and use preset-env with a proper useBuiltIns config

Can we give that a try?

@skjnldsv skjnldsv modified the milestones: 1.1.1, 1.2.1 Nov 21, 2019
@raimund-schluessler
Copy link
Contributor

So what do I have to do to get nextcloud/tasks#688 running?

@skjnldsv
Copy link
Contributor

Fix in #749

@ChristophWurst ChristophWurst deleted the bugfix/691/babel-runtime branch November 27, 2019 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Babel modules not found
5 participants