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

Release two builds of the project #603

Closed
wants to merge 2 commits into from

Conversation

alcuadrado
Copy link
Member

This PR prepares the tooling to release two different builds (ES5 & ES2017) of the project in the same NPM package.

I chose the name "modernjs" to avoid using a specific ES version, as we may want to revisit which version we target in the future.

@coveralls
Copy link

coveralls commented Sep 20, 2019

Coverage Status

Coverage remained the same at 96.029% when pulling ce879d1 on two-builds-release-process into 00d8fe3 on master.

@alcuadrado
Copy link
Member Author

I think we should add a comment about ES2017 not be a definite decision, and that we may change it in the future.

I think a sane criterion can be to set it to the oldest ES version supported by actively maintained node versions.

Updating it when used in the browser is a little different, because:

  1. You need to have a proper bundler in place.
  2. Changing the ES version may break some build and require some config tweaking, but it won't break any user-facing app.

@davidmurdoch what do you think?

@davidmurdoch
Copy link
Contributor

Truffle has an "unofficial" plan to maintain Node 8 compatibility until the major cloud providers (like Google Cloud Function) offer stable versions of Node 10 (and Google Cloud Functions does not).

Have you looked in to the TypeScript --lib option? https://www.typescriptlang.org/docs/handbook/compiler-options.html I haven't read too much on how it works, but it may allow for us to target ES6 + async/await? I'm not even sure if this is a use-case --lib can handle, I only remember reading about it a while back.

I've also seen the try/catch approach to attempt to include modern ES syntax before defaulting to a fallback, which would look something like this:

// ./dist/index.js

let lib;
try {
  lib = require("./es2017/index.js");
} catch(e) {
  lib = require("./es5/index.js");
}
/* export lib... */

@alcuadrado
Copy link
Member Author

Truffle has an "unofficial" plan to maintain Node 8 compatibility until the major cloud providers (like Google Cloud Function) offer stable versions of Node 10 (and Google Cloud Functions does not).

That's interesting. I couldn't find any info about their Node.js versions support policy.

We don't need to define a policy right no though. I just think that we should make it clear that the users of the "modernjs" build should expect it to be updated when reasonable. And maybe link an issue to discuss what's reasonable.

I wouldn't wait for this to be settled before releasing, as we can always fall back to just keep ES2017.

Have you looked in to the TypeScript --lib option? https://www.typescriptlang.org/docs/handbook/compiler-options.html I haven't read too much on how it works, but it may allow for us to target ES6 + async/await? I'm not even sure if this is a use-case --lib can handle, I only remember reading about it a while back.

We could do that, but ES6 + async/await is almost the same than ES2017. These are the proposals that ES2017 included.

ES2017 is fully supported by Node 8.

I've also seen the try/catch approach to attempt to include modern ES syntax before defaulting to a fallback, which would look something like this:

I think this won't work here, as when using the VM it's normal to import deeply nested submodules, like:

import { ExecResult } from "ethereumjs-vm/dist/evm/evm";
import { ERROR } from "ethereumjs-vm/dist/exceptions";
import { RunBlockResult, TxReceipt } from "ethereumjs-vm/dist/runBlock";
import { StateManager } from "ethereumjs-vm/dist/state";
import PStateManager from "ethereumjs-vm/dist/state/promisified";

Doing this conditional require in the index file will lead to mixing both builds. That would be confusing at least, and maybe break things.

@s1na
Copy link
Contributor

s1na commented Sep 23, 2019

(Sorry unrelated to general convo)

I think this won't work here, as when using the VM it's normal to import deeply nested submodules, like:

IMO this should change and users shouldn't rely on directory structure/filenames. We should export all things we want to expose publicly in index.

@s1na
Copy link
Contributor

s1na commented Sep 23, 2019

Another question I had when thinking about this is when do we want to consider targeting ES6 as the default build? It seems only IE11 is lagging behind, but I don't think they'll ever catch up. All other browsers have (almost) full support for ES6: https://kangax.github.io/compat-table/es6/

@alcuadrado
Copy link
Member Author

alcuadrado commented Sep 23, 2019

IMO this should change and users shouldn't rely on directory structure/filenames. We should export all things we want to expose publicly in index.

I'm not sure if I agree. Many projects are moving to direct/nested imports because that works nicer with web bundlers.

I don't have an opinion about this project in particular. I would have to pay more attention to how it's used and bundler to have one. There are projects where not every file/feature is used all the time, and those are better suited to nested imports. Not sure if it's the case here.

Another question I had when thinking about this is when do we want to consider targeting ES6 as the default build? It seems only IE11 is lagging behind, but I don't think they'll ever catch up. All other browsers have (almost) full support for ES6: https://kangax.github.io/compat-table/es6/

I'm afraid the tooling is the problem now. Many things have the implicit requirement that npm modules should be ES5, and of course, CJS. I think you'd find this repo of mine interesting.

@holgerd77
Copy link
Member

I have mixed feelings with this, I think we might not want to throw these updates here too quickly out of the door. No one is helped if we encourage people to use the "modern" version of the library which is then tempting and at the end it will eventually break their stuff.

I would suggest to keep this a bit open in some discussion/WIP state and do some more experimentation. We should also first see how the other performance improvements @alcuadrado realized already will play out in a real world (TM) Truffle scenario.

But nevertheless: think it's very valuable to have some more experimentation on the build side, think we have pretty much little experience here.

@alcuadrado alcuadrado force-pushed the two-builds-release-process branch from 79139c8 to ce879d1 Compare September 26, 2019 22:39
@alcuadrado
Copy link
Member Author

After thinking more about this, I've reconsidered. There's not much to gain by aggressively updating the target ES version beyond ES2017. I think we should stick to ES2017 at least during the 4.x lifecycle.

I updated this pr to reflect that, by changing the name modenjs to es2017.

@evertonfraga
Copy link
Contributor

evertonfraga commented Nov 20, 2019

Bumping the conversation :)

CI tests now run against /dist. Naturally, they should expand to /es2017. On a whitelisting approach, what would be the relevant test scopes to ensure a good degree safety? We don't want the CI to 💣💥.

On an additional note, several test files seem to require directly from /dist, that should be changed to a dynamic approach.

@ryanio
Copy link
Contributor

ryanio commented Sep 21, 2020

This will be done in #886 with the v2 typescript config, which outputs dist.browser with the addition of a tsconfig.browser.json (see usage)

@ryanio ryanio closed this Sep 21, 2020
@ryanio ryanio deleted the two-builds-release-process branch October 28, 2021 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants