Skip to content
This repository has been archived by the owner on Oct 4, 2022. It is now read-only.

Build via webpack #1687

Closed
igorschoester opened this issue Aug 8, 2018 · 4 comments
Closed

Build via webpack #1687

igorschoester opened this issue Aug 8, 2018 · 4 comments

Comments

@igorschoester
Copy link
Member

Current situation

The build is done via grunt. Which actually build all the JavaScript with the typescript builder. (and twice because of the templates).

Wanted situation

Use webpack. This will allow us to specify loaders for each type.

How we found out:
With the webworker we found out about this because we kept running into require / import problems. The code generated by TypeScript is not having a helper around the fact that a require could live inside a .default space. Which webpack does do properly. Also, the worker is created with the assumption of the worker-loader, which is not currently being done in the built. So the js file created does not work on the fly.

@igorschoester
Copy link
Member Author

igorschoester commented Aug 29, 2018

TypeScript removal is done in #1745
Imports are done in #1737

So probably not much left, and not sure webpack is the solution for this library. Therefore, closing this issue.

@Kingdutch
Copy link
Contributor

Kingdutch commented Oct 22, 2018

Hi igorschoester,

I don't know if this is the right issue to post this in or whether we should open a new issue.

For the Drupal module that uses your library I'm trying to compile YoastSEO using Webpack. The imports are a good first start (I did run into #1878 ). However, building this with Webpack and Babel is currently impossible because the project requires keeping too many files in memory at the same time.

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0x10003907e node::Abort() [/usr/local/Cellar/node/10.11.0/bin/node]

I've tried converting all the require statements in my own code into proper import statements but I suspect that Webpack is choking on all the require statements throughout the project.

Are there any plans to convert the internal code for YoastSEO to also use import statements instead of require statements? If you're aware of a different solution without conversion then that would also be great.

You can check out the out of memory error by cloning https://github.com/goalgorilla/RTSEO.js/tree/feature/improved-build-pipeline and running yarn && yarn build to install dependencies and start Webpack in production mode.

EDIT:
I just realised while looking at your repository that you already made this step for all files. Before this I was looking at the files that were installed through NPM.

It appears that in the current situation the version that's published on NPM exposes an index file that uses ES6 style exports and thus require transpilation. However, all the internals on NPM are already compiled which causes a huge overhead when trying to transpile again using Babel (I initially experienced this with babelify before trying Webpack with babel-loader).

Letting Yarn install directly from the Github repository so that the original ES6 source is available allows the compilation to work as expected 👍 It would be nice if the original ES6 source could also be available through NPM (I'm not sure how other projects such as React handle this).

The following issues still need to be resolved however (they are output as Webpack warnings):

src/assessments/seo/subheadingsKeywordAssesment.js uses module.exports instead of export default

~ Alexander

Kingdutch added a commit to goalgorilla/RTSEO.js that referenced this issue Oct 22, 2018
Babel can't handle files that have been compiled previously by
Babel because it contains too much duplication. Luckily the source
of the YoastSEO library is still available in the repository which
Babel can use with webpack to compile just fine.

See Yoast/YoastSEO.js#1687 (comment)
@Kingdutch
Copy link
Contributor

The module.exports to export default is fixed in #1919

@atimmer
Copy link
Contributor

atimmer commented Oct 30, 2018

@Kingdutch #1919 covers your short-term issue. I've put that in our process to review.

#1930 should cover the longer term issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants