Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

refactor: use serialize-javascript instead own implementation #183

Merged
merged 1 commit into from
Dec 8, 2017

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Dec 5, 2017

Notable Changes

  1. serialize-javascript more stable and support many features (see https://github.com/yahoo/serialize-javascript/blob/master/index.js#L14 https://github.com/yahoo/serialize-javascript/blob/master/index.js#L90 and etc)
  2. Reducing the code, serialization out of scope plugin code and should be separate package

Issues

  • None

@michael-ciniawsky michael-ciniawsky changed the title refactor: use serialize-javascript package instead own implementation refactor: use serialize-javascript instead own implementation Dec 7, 2017
@michael-ciniawsky
Copy link
Member

Nothing against this change in particular and if serialization is unavoidable, but I would like to traige eventual possibilities to avoid serialization for options altogether if possible (maybe it isn't).

  1. webpack uses an ident, maybe we can use a similiar approach
  2. https://github.com/rvagg/node-worker-farm#exportedmethods but not 💯 if this is helpful here (didn't tested it out yet)

@alexander-akait
Copy link
Member Author

@michael-ciniawsky ident don't contain function, regex and date, we can't use this as cache key


module.exports = (options, callback) => {
try {
callback(null, minify(JSON.parse(options, decode)));
// eslint-disable-next-line no-eval
callback(null, minify(eval(`(${options})`)));
Copy link
Member

@michael-ciniawsky michael-ciniawsky Dec 7, 2017

Choose a reason for hiding this comment

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

// 'use strict' => this === undefined (Clean Scope)
// Safer for possible security issues, albeit not critical at all here 
options = new Function(`'use strict'\nreturn ${options}`)()

callback(null, minify(options));

@@ -378,11 +379,12 @@ describe('when options.cache', () => {

// Make sure that we cached files
expect(cacheEntriesListKeys.length).toBe(files.length);
cacheEntriesListKeys.forEach((cacheJSONEntry) => {
const cacheEntry = JSON.parse(cacheJSONEntry);
cacheEntriesListKeys.forEach((serializedCacheEntry) => {
Copy link
Member

Choose a reason for hiding this comment

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

cacheEntriesListKeys => cacheKeys && serializedCacheEntry => cacheEntry

@@ -106,7 +106,7 @@ exports[`ecma 6: errors 1`] = `Array []`;

exports[`ecma 6: main.js 1`] = `
"webpackJsonp([ 0 ], [ function(module, exports) {
class Mortgage {
module.exports = class {
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change ? It's the same as in #190 I just can't reason about it 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky oh yep 👍

@michael-ciniawsky michael-ciniawsky added this to the 1.1.2 milestone Dec 7, 2017
@alexander-akait alexander-akait force-pushed the refactor-use-serialize-javascript branch 2 times, most recently from 75a3454 to ea2c021 Compare December 8, 2017 09:57
@alexander-akait
Copy link
Member Author

@michael-ciniawsky done

module.exports = (options, callback) => {
/* eslint-disable no-new-func */

module.exports = (workerOptions, callback) => {
Copy link
Member

@michael-ciniawsky michael-ciniawsky Dec 8, 2017

Choose a reason for hiding this comment

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

workerOptions => options (Just reassign options below please)

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky eslint throw problem about no-param-reassign, but it is not make sense here.

Copy link
Member

Choose a reason for hiding this comment

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

eslint-disable-next-line ? 🙃

@alexander-akait alexander-akait force-pushed the refactor-use-serialize-javascript branch from ea2c021 to eeee677 Compare December 8, 2017 11:31
@alexander-akait
Copy link
Member Author

@michael-ciniawsky done 👍

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

Successfully merging this pull request may close these issues.

2 participants