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

Should built-in modules be deeply frozen? #15

Open
littledan opened this issue Dec 17, 2018 · 20 comments
Open

Should built-in modules be deeply frozen? #15

littledan opened this issue Dec 17, 2018 · 20 comments

Comments

@littledan
Copy link
Member

This proposal repository mentions that built-in modules should be frozen, so they can be reliably accessed. This idea is different from how web (and ES modules by default) works today, where you can monkey-patch existing platform classes.

Although virtualizing with something like import-maps can address some use cases, it wouldn't let a platform-vended object have additional/replaced methods the way that monkey-patching can. Is this an important issue? Are there any solutions while maintaining reliability?

Related: #13

@ljharb
Copy link
Member

ljharb commented Dec 17, 2018

This is an important issue - it’s critical to me that all builtins be able to be brought up to a later spec and/or replaced. Additionally, being able to add new functionality, or repair broken functionality, without having to reimplement the entire feature, keeps complexity and bundle sizes down.

@leobalter
Copy link
Member

@ljharb for clarification, do you mean freezing the built-in modules would prevent the advantages you're mentioning?

Additionally, being able to add new functionality, or repair broken functionality, without having to reimplement the entire feature,

Here I understand you want them extensible through new feats and patches, so the frozen modules would avoid those.


On my PoV, having them frozen sounds nice for reliability, but extensibility is great as well and matches a what JS have been since forever. I'd love to know more arguments.

@littledan
Copy link
Member Author

One proposed pattern, within the frozen-modules world, was that you could import a module, extend a class defined in it, and re-export it, rather than modifying the class. Would that work for the cases that people are concerned about?

@ljharb
Copy link
Member

ljharb commented Dec 17, 2018

Yes, freezing anything built-in prevents those advantages.

Subclassing is not sufficient, because that is observably different from the builtin. The intention is to unobservably provide the modified/replaced/added builtin to all code.

@erights
Copy link

erights commented Dec 17, 2018

This is indeed extremely important. EcmaScript builtin modules should be viewed as extensions of the primordials into the module namespace. They are implicitly shared by everything sharing that realm/loader. They must preserve the essential virtues of the primordials:

  • No hidden mutable state, connectivity, or I/O ability (with three patchable grandfathered exceptions). By "hidden", I mean: That remains after all the primordial objects are frozen.
  • Almost all properties are mutable, enabling the primordials to be shimmed before freezing.

Preserving these virtues is actually difficult to do because of the differences between modules and primordials.

For modules, these two requirements are in tension. I know of three ways of resolving the tension:

  • Have builtin modules in fact start as transitively frozen, which satisfies the first requirement trivially, but makes approaches to the second awkward and imperfect. (Inherit with overridden differences. Mostly compat with most code, but not transparent and will thus break some reflective code.)
  • Have builin modules start without hidden state, like the primordials, but with many mutable properties, like the primordials, but arranged such that all mutable state can be both mutated and frozen starting only from the exports and doing only reflective property navigation (getOwnPropertyDescriptor rather than [[Get]]).
  • Adding a new deep-freeze operation to the platform, likely in the Realms API, for deep-freezing all primordial state after all shimming has been completed.

@ljharb
Copy link
Member

ljharb commented Dec 17, 2018

I think it’s worth noting that any solution like @erights has proposed would work equally well with or without builtin modules, and might want to be pursued first - so that it’s available for new globals and for builtin modules.

@leobalter
Copy link
Member

Adding a new deep-freeze operation to the platform, likely in the Realms API, for deep-freezing all primordial state after all shimming has been completed.

@erights From a developer PoV this feels such like great feature to have. Basically an opt-in switch to turn this feature one and it can't be turned off in the same Realm/load.

This would be probably be highly appreciated by a large community of function programming developers as well, without causing harm to those willing to use it.

@Mouvedia
Copy link

Mouvedia commented Dec 17, 2018

There are performance implication to a feature-flipping approach.
e.g. reimplementation of builtins that don't follow the spec to the T and hence are faster

Ignoring those corner cases on a global scale would indeed be quite appealing.

@erights
Copy link

erights commented Dec 17, 2018

Hi @Mouvedia , I'm not sure I understand. Could you expand? Thanks.

@Mouvedia
Copy link

Mouvedia commented Dec 17, 2018

Basically an opt-in switch to turn this feature one and it can't be turned off in the same Realm/load.

@erights I interpret that as some sort of a feature flipping for the developer. Is this what he meant?
Concerning the reimplementations, check fast.js. In short if you cut some corners things get faster and web developers do like that.

@chicoxyzzy
Copy link
Member

chicoxyzzy commented Dec 17, 2018

IMO all std things should be frozen. There are plenty of tries of patching built-ins and this only creates more web compat issues (and potentially performance issues). If something is broken in particular browser std lib there should be another way to fix it.

@ljharb
Copy link
Member

ljharb commented Dec 17, 2018

@chicoxyzzy short of full replacement (which might require a full wrapper, or reimplementation), what would be another way to fix something that's broken?

@chicoxyzzy
Copy link
Member

@ljharb I have no any proposals in mind yet, maybe special syntax, IDK. Patching std lib could lead to problems like some proposal (contains, flatMap, global, etc.) faced before.

@ljharb
Copy link
Member

ljharb commented Dec 17, 2018

I suspect that polyfilling and deniability requirements will conflict with solving the web compatibility problem; for prototype methods (like contains, flatMap, includes, etc), it seems like there's not a solution at all in the absence of a call operator.

@chicoxyzzy
Copy link
Member

Yes, that's a problem definitely. How other interpreted languages solve these problems?

@chicoxyzzy
Copy link
Member

chicoxyzzy commented Dec 17, 2018

It seems that it's easier for most of other languages because they are interpreted in concrete environments but maybe there are some other embedded languages that have separate targets to be interpreted on?

@tastypackets
Copy link

tastypackets commented Dec 18, 2018

Referencing polyfilling of the stds, could we allow the developer to toggle the polyfill even if the std is supported? The developer could intentially utilize the polyfill on browsers with a broken builtin std and accept the negative performance to avoid using broken builtin std lib.

Edit:
Looks like this was already discussed on the Polyfill issue.

#2

@littledan
Copy link
Member Author

I'm pretty skeptical of freezing the built-in modules because it's not clear to me that this wouldn't regress the platform's virtualization capabilities, as @bakkot explained was important in domenic/get-originals#18 . I think we should pursue modules which are mutable by default, like everything today, and in parallel, a (virtualizable) mechanism to get at the original built-ins, as discussed in https://github.com/domenic/get-originals and #13 .

@mattijs
Copy link
Member

mattijs commented Dec 18, 2018

I would like to aim for freezing the exports from a built in module, although I'm not quite sure how yet. There are a lot of pitfalls to navigate.

@jsumners
Copy link

jsumners commented Jan 4, 2019

Why would a standard library be implemented contrary to the fundamental feature of the language, i.e. mutability via the prototype? I don't care for libraries that patch the built-in prototypes, but there is a use case I see as valid -- testing. For example:

https://github.com/pinojs/pino/blob/ef50bb3c606b1ce94e672d301e7d5d1f9310e96e/test/timestamp.test.js#L76-L89

test('pino.stdTimeFunctions.unixTime returns seconds based timestamps', async ({ is }) => {
  const opts = {
    timestamp: pino.stdTimeFunctions.unixTime
  }
  const stream = sink()
  const instance = pino(opts, stream)
  const now = Date.now
  Date.now = () => 1531069919686
  instance.info('foobar')
  const result = await once(stream, 'data')
  is(result.hasOwnProperty('time'), true)
  is(result.time, 1531069920)
  Date.now = now
})

If Date were frozen, this test would be tricker since it is testing a function that internally invokes Date.now and we want a known value from that method for the test.

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

No branches or pull requests

9 participants